Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make the "partial molar volume" a function of stoichiometry #2943

Closed
wants to merge 12 commits into from

Conversation

aabills
Copy link
Contributor

@aabills aabills commented May 10, 2023

Description

For many chemistries, notably high-nickel NMCs and graphite, the volume change associated with intercalation is a strong function of stoichiometry. Here, I have taken the r-averaged value for Ω and used it to calculate total stress. This accounts for the change without violating the initial model assumptions.

Type of change

Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #) - note reverse order of PR #s. If necessary, also add to the list of breaking changes.

  • New feature (non-breaking change which adds functionality)
  • Optimization (back-end change that speeds up the code)
  • Bug fix (non-breaking change which fixes an issue)

Key checklist:

  • No style issues: $ pre-commit run (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
  • All tests pass: $ python run-tests.py --all
  • The documentation builds: $ python run-tests.py --doctest

You can run unit and doctests together at once, using $ python run-tests.py --quick.

Further checks:

  • Code is commented, particularly in hard-to-understand areas
  • Tests added that prove fix is effective or that feature works

@codecov
Copy link

codecov bot commented May 10, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ebacf49) 99.59% compared to head (bf6e83b) 99.71%.

❗ Current head bf6e83b differs from pull request most recent head cfb467f. Consider uploading reports for the commit cfb467f to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #2943      +/-   ##
===========================================
+ Coverage    99.59%   99.71%   +0.11%     
===========================================
  Files          257      273      +16     
  Lines        20639    19007    -1632     
===========================================
- Hits         20556    18952    -1604     
+ Misses          83       55      -28     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@valentinsulzer valentinsulzer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

There is already a parameter, t_change, for the electrode thickness change as a function of particle concentration. Should that be the same parameter?

@aabills
Copy link
Contributor Author

aabills commented May 11, 2023

They are closely related, but not the same. In most cases, t_change is a function of Omega, but t_change is more of a summary variable to monitor volume changes of the cell while Omega is used to calculate the stress within a particle. According to Appendix B of Ai 2020 (which based on comments is the paper around which pybamm's stress models are implemented), this is to avoid numerical difficulties.

For example, t_change in the Ai2020 parameter set for the positive electrode is Ω*c_s_max*stoich, but it is a more complicated function for the negative electrode.

@aabills
Copy link
Contributor Author

aabills commented May 11, 2023

I think we could get rid of t_change with this, but it would require us to change the parameter sets to be different from the papers from which they were created.

@aabills aabills marked this pull request as draft May 15, 2023 21:24
@aabills
Copy link
Contributor Author

aabills commented Nov 29, 2023

Closed in favor of #3576

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants