Better optimizer for initializing half-cell soc #4873
Conversation
of scipy.opimize.root() to resolve numerical crashes.
d-cogswell
force-pushed
the
issue/4868
branch
from
99fbd15 to
9422888
Compare
|
Thanks, @d-cogswell – I've triggered the workflows for you. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4873 +/- ##
========================================
Coverage 98.70% 98.70%
========================================
Files 304 304
Lines 23483 23483
========================================
Hits 23180 23180
Misses 303 303 ☔ View full report in Codecov by Sentry. |
d-cogswell
changed the title
There was a problem hiding this comment.
Thanks @d-cogswell, largely this looks good and works for me as well, I've got a couple of questions below that should be addressed
| @@ -31,7 +31,7 @@ def __init__(self, name="ElectrodeSOH model"): | |||
| Q_w = pybamm.InputParameter("Q_w") | |||
| T_ref = param.T_ref | |||
| U_w = param.p.prim.U | |||
| Q = Q_w * (x_100 - x_0) | |||
| Q = Q_w * (x_0 - x_100) | |||
There was a problem hiding this comment.
why was Q altered here?
There was a problem hiding this comment.
@valentinsulzer noticed that Q was negative for half-cells, and this change addresses that. I happened to be looking through this part of the code, I discovered it was defined incorrectly for a positive electrode. x_0 is the stoichiometry at 0% SOC, and x_100 at 100% SOC. For a cathode x_0 > x_100.
| @@ -49,13 +49,12 @@ def __init__(self, name="ElectrodeSOH model"): | |||
| "Uw(x_100)": U_w(x_100, T_ref), | |||
| "Uw(x_0)": U_w(x_0, T_ref), | |||
| "Q_w": Q_w, | |||
| "Q_w * (x_100 - x_0)": Q_w * (x_100 - x_0), | |||
There was a problem hiding this comment.
why was this variable removed?
There was a problem hiding this comment.
This expression is also negative, so I didn't see a reason to keep it. A half-cell only needs on Q variable.
| } | ||
|
|
||
| @property | ||
| def default_solver(self): | ||
| # Use AlgebraicSolver as CasadiAlgebraicSolver gives unnecessary warnings | ||
| return pybamm.AlgebraicSolver() | ||
| return pybamm.AlgebraicSolver(method="minimize L-BFGS-B", tol=1e-7) |
There was a problem hiding this comment.
I tried this out on the example linked in your issue. Using the default tol (1e-6) seems to work reliably for me, is there a reason to tighten this?
There was a problem hiding this comment.
I changed the tolerance because a regression test was failing.
Description
scipy.opimize.root()occasionally causes numerical crashes during initialization of half-cell simulations. Switching toscipy.optimize.minimize()resolves the issue.Fixes #4868
Type of change
Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #)
Important checks:
Please confirm the following before marking the PR as ready for review:
nox -s pre-commitnox -s testsnox -s doctests