DFN with particle size distributions#1602
DFN with particle size distributions#1602valentinsulzer merged 22 commits intopybamm-team:developfrom
Conversation
…ze-distribution-DFN
Codecov Report
@@ Coverage Diff @@
## develop #1602 +/- ##
===========================================
+ Coverage 97.97% 97.99% +0.01%
===========================================
Files 324 327 +3
Lines 18521 18665 +144
===========================================
+ Hits 18146 18290 +144
Misses 375 375
Continue to review full report at Codecov.
|
| if particle_side == "Fickian diffusion": | ||
| self.submodels[ | ||
| domain.lower() + " particle" | ||
| ] = pybamm.particle.FickianManyParticles(self.param, domain) |
There was a problem hiding this comment.
can we rename these submodels? the "many" here is confusing. maybe XDistribution/XAveraged?
There was a problem hiding this comment.
Yeah the whole naming convention with "many" and "single" is getting more cumbersome/confusing, I agree. Instead of making the class names longer, I've gone with submodules size_distribution and no_distribution (could do size_averaged instead) to differentiate them. Then the class names indicate just the transport model (Fickian, polynomial profile, etc) and whether it is "x-averaged". For example,
particle.FickianManyParticles is now particle.no_distribution.FickianDiffusion
particle.FickianSingleParticles is now particle.no_distribution.XAveragedFickianDiffusion
particle.FickianManySizeDistributions is now particle.size_distribution.FickianDiffusion
particle.FickianSingleSizeDistribution is now particle.size_distribution.XAveragedFickianDiffusion
Could also maybe group them into x_averaged and x_full submodules in a similar way, if that would make it even clearer. (Easily changed)
| [self.domain.lower() + " particle"], | ||
| { | ||
| "secondary": self.domain.lower() + " particle size", | ||
| "tertiary": self.domain.lower() + " electrode", |
There was a problem hiding this comment.
Yep, thanks, this should be x-averaged.
| import unittest | ||
|
|
||
|
|
||
| class TestManySizeDistributions(unittest.TestCase): |
There was a problem hiding this comment.
getting rid of these submodel tests anyway so no need to add
There was a problem hiding this comment.
Ok, removed them (I modified the existing particle submodel tests just so that they pass, didn't bother to fully rename everything there).
2 participants
Description
Added two submodels (
FickianManySizeDistributionsandFastManySizeDistributions) to enable size distributions to be used in the DFN model, as in this paper. These are analogous to theFickianSingleSizeDistributionandFastSingleSizeDistributionsubmodels (used for the MPM) but including x-dependence. Features accessed usingoptions = {"particle size": "distribution"}.Also added an example notebook
DFN-with-particle-size-distributions.ipynband script.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.
Key checklist:
$ flake8$ python run-tests.py --unit$ cd docsand then$ make clean; make htmlYou can run all three at once, using
$ python run-tests.py --quick.Further checks: