-
-
Notifications
You must be signed in to change notification settings - Fork 553
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
Polynomial concentration #1130
Polynomial concentration #1130
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1130 +/- ##
===========================================
+ Coverage 97.90% 97.92% +0.02%
===========================================
Files 247 247
Lines 13750 13910 +160
===========================================
+ Hits 13462 13622 +160
Misses 288 288
Continue to review full report at Codecov.
|
self.initial_conditions.update({c_s_surf: c_init}) | ||
if self.order == 4: | ||
# We also need to provide an initial condition (initial guess for the | ||
# algebraic solver) for the average concentration gradient |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove the comment about algebraic solver here, as it is an ODE for q (accidentally left the comment here from copy/paste above)
(5 / 2) * (c_s_surf_xav - c_s_rxav), [self.domain.lower() + " particle"] | ||
) | ||
if self.domain == "Negative": | ||
# TODO: figure out how to just use r from standard_spatial_vars |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I kept getting shape errors here and couldn't figure out the correct way to broadcast things so I could use r
from standard_spatial_vars
here. I thought I should broadcast A, B, (C), then use that to define c_s
, and then average to get c_s_av
, but I kept getting bogged down in shape errors
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since c_s_xav
doesn't depend on x
, it makes sense that you should use a different r
which only has "negative particle" and "current collector" as domains
|
||
self.initial_conditions = {c_s_rxav: c_init} | ||
if self.order == 4: | ||
# We also need to provide an initial condition (initial guess for the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see above comment about removing the comment about algebraic solvers here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @rtimms , looks good to me!
] = pybamm.particle.PolynomialManyParticles(self.param, "Negative", order) | ||
self.submodels[ | ||
"positive particle" | ||
] = pybamm.particle.PolynomialManyParticles(self.param, "Positive", order) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here it makes more sense to pass the name of the option rather than the order. Otherwise one might naively wonder why there are no orders 1 and 3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree
@@ -49,6 +66,10 @@ def _get_standard_concentration_variables(self, c_s, c_s_xav): | |||
"R-averaged " | |||
+ self.domain.lower() | |||
+ " particle concentration [mol.m-3]": c_s_rav * c_scale, | |||
"R-X-averaged " + self.domain.lower() + " particle concentration": c_s_av, | |||
"R-X-averaged " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we have been using "Average" to refer to "R-X-averaged". Do you think this is clearer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy to change to "Average". I think it is obvious and less cumbersome.
(5 / 2) * (c_s_surf_xav - c_s_rxav), [self.domain.lower() + " particle"] | ||
) | ||
if self.domain == "Negative": | ||
# TODO: figure out how to just use r from standard_spatial_vars |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since c_s_xav
doesn't depend on x
, it makes sense that you should use a different r
which only has "negative particle" and "current collector" as domains
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey Rob, thanks for this, seems like quite a lot of work in the end. Looks great and works well. I added another notebook for the models which included comparing at different C-rates. Feel free to change bits or move it to another folder. I did get some ugly solver errors in the notebook at 5 A which I guess we could hide but are they anything to worry about? Still gets a solution...
thanks @TomTranter notebook looks great! made some minor adjustments. the solver errors were just because of the high current - the solver was taking steps/doing jacobian calculations that were later rejected and the step size decreased. in practice they are nothing to worry about, but I've changed the currents in the notebooks to avoid the warnings and still show off the same results. probably best to avoid potentially confusing solver errors in the examples |
Description
Adds particle submodels that approximate the concentration profile within the particle as a polynomial in r. Note the "fast diffusion" option is now accessed as the "uniform profile" option (fast diffusion leads to uniform concentration in r, but it makes sense to have uniform concentration as the zero-order case of the polynomial model).
Fixes #1128
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 docs
and then$ make clean; make html
You can run all three at once, using
$ python run-tests.py --quick
.Further checks: