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

Many-Particle Models #1529

Merged
merged 82 commits into from
Jul 29, 2021
Merged

Many-Particle Models #1529

merged 82 commits into from
Jul 29, 2021

Conversation

tobykirk
Copy link
Contributor

@tobykirk tobykirk commented Jun 30, 2021

Description

This branch adds the Many-Particle Model or MPM, a full lithium-ion battery model including a distribution of particle sizes in each electrode. The particle size is implemented as an additional "dimension", where the user specifies the continuous particle-size distribution which is then discretized. The model, it's features and how to use it are detailed in the notebook examples/notebooks/MPM.ipynb.

Included is a new submodel module particle.size_distribution with 2 submodels FickianSingleSizeDistribution and FastSingleSizeDistribution (and a base distribution model) used by the MPM. These are the "X-averaged" ones, with submodels incorporating x-variation (FickianManySizeDistributions, etc.) to be merged later (pending the ability to have 4 domains). Changes to the existing code are currently quite minimal (probably to a fault!), mainly adding functionality where possible.

The branch includes all the base functionality for the new "particle size" domain/dimension, including additions to

  • standard spatial variables (R_n and R_p)
  • standard battery geometry
  • lithium ion parameter class (e.g. area-weighted size distributions f_a_dist_n, f_a_dist_p)
  • BaseInterface and BaseKinetics (where special treatment of variables that depend on "particle size" is needed)
  • ProcessedVariable
  • QuickPlot
  • an R_average() operator, for convenient particle-size averaging

Not currently compatible with the degradation models, i.e., SEI, particle cracking/swelling, lithium plating - they are not allowed as options.

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: $ flake8
  • All tests pass: $ python run-tests.py --unit
  • The documentation builds: $ cd docs and then $ make clean; make html

You can run all three 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

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, looks great now!

pybamm/parameters/size_distribution_parameters.py Outdated Show resolved Hide resolved
@valentinsulzer
Copy link
Member

You can merge develop and push again to trigger the tests with the fix to new_copy

elif self.domain == "Positive":
N_s_xav_distribution = -self.param.D_p(
c_s_xav_distribution, T_k_xav
) * pybamm.grad(c_s_xav_distribution) / R
) * pybamm.grad(c_s_xav_distribution)
Copy link
Member

Choose a reason for hiding this comment

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

why this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just to make the definition of the nondimensional flux consistent with FickianManyParticles (where you can have a size distribution in x), and also the scaling used for the MP-DFN in our recent JES paper. The rhs is unchanged, as the factor of / R is just moved to lines 166 and 172, turning an / R into / R ** 2. Should make it easier to combine the "distribution" and "no distribution" submodels later on.

Meant to make this change much earlier, but seems I forgot. Sorry for the confusion here!

@codecov
Copy link

codecov bot commented Jul 16, 2021

Codecov Report

Merging #1529 (34700c0) into develop (c07c7dc) will increase coverage by 0.05%.
The diff coverage is 100.00%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1529      +/-   ##
===========================================
+ Coverage    97.78%   97.84%   +0.05%     
===========================================
  Files          323      329       +6     
  Lines        17552    17987     +435     
===========================================
+ Hits         17164    17599     +435     
  Misses         388      388              
Impacted Files Coverage Δ
...ttery_models/lithium_ion/base_lithium_ion_model.py 100.00% <ø> (ø)
...nterface/inverse_kinetics/inverse_butler_volmer.py 100.00% <ø> (ø)
pybamm/models/submodels/particle/base_particle.py 100.00% <ø> (ø)
pybamm/__init__.py 95.00% <100.00%> (+0.04%) ⬆️
pybamm/expression_tree/broadcasts.py 100.00% <100.00%> (ø)
pybamm/expression_tree/unary_operators.py 100.00% <100.00%> (ø)
pybamm/geometry/battery_geometry.py 100.00% <100.00%> (ø)
pybamm/geometry/standard_spatial_vars.py 100.00% <100.00%> (ø)
...m/models/full_battery_models/base_battery_model.py 99.70% <100.00%> (+0.01%) ⬆️
...models/full_battery_models/lithium_ion/__init__.py 100.00% <100.00%> (ø)
... and 22 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c07c7dc...34700c0. Read the comment docs.

@tobykirk
Copy link
Contributor Author

To describe the last few commits: I've added a few more tests to increase the coverage, particularly for processed_variable.py (testing variables that depend on particle size), broadcasts.py (to/from size domain) and mpm.py (testing the default parameter values and necessary options).

For the processed_variable tests, I needed to modify get_mesh_for_testing() and get_discretization_for_testing(). (Maybe the tests here get repetitive and could be shortened, though?)

@tobykirk
Copy link
Contributor Author

tobykirk commented Jul 23, 2021

Not passing the total coverage check as I think codecov is comparing to an older commit (f83f0a2) rather than e.g. 396c9ef ? @tinosulzer

@valentinsulzer
Copy link
Member

Yeah we could definitely reduce some duplication in the processed variable tests

@tobykirk
Copy link
Contributor Author

Yeah we could definitely reduce some duplication in the processed variable tests

Had a go at reducing the duplication by moving code into a method process_and_check_2D_variable (x-r, r-R, R-x, x-z, R-z variables are all tested almost identically)

@valentinsulzer valentinsulzer merged commit ef21abe into pybamm-team:develop Jul 29, 2021
@valentinsulzer
Copy link
Member

@all-contributors add @tobykirk for ideas, code, tests

@allcontributors
Copy link
Contributor

@tinosulzer

I've put up a pull request to add @tobykirk! 🎉

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