Add pybammsolvers nonlinear solver#5459
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5459 +/- ##
==========================================
- Coverage 98.29% 98.15% -0.15%
==========================================
Files 337 338 +1
Lines 30950 31119 +169
==========================================
+ Hits 30423 30544 +121
- Misses 527 575 +48 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@martinjrobins on my machine, this fixes the |
BradyPlanden
left a comment
BradyPlanden
left a comment
There was a problem hiding this comment.
This looks great - thanks @MarcBerliner! Just a few comments/questions
| "Topic :: Scientific/Engineering", | ||
| ] | ||
| dependencies = [ | ||
| "pybammsolvers>=0.6.0,<0.7.0", |
There was a problem hiding this comment.
issue: I think it's worth removing the upper pin on pybammsolvers. I just realised we haven't been using 0.7.0 since it wasn't updated here. It probably better to fail loudly, then to silently forget about it.
| alg_res_fn = None | ||
| jac_alg_fn = None | ||
|
|
||
| if alg_res_fn is None or not callable(getattr(alg_res_fn, "serialize", None)): | ||
| alg_res_fn = casadi.Function("empty_alg_res", [], []) | ||
| if jac_alg_fn is None or not callable(getattr(jac_alg_fn, "serialize", None)): | ||
| jac_alg_fn = casadi.Function("empty_alg_jac", [], []) |
There was a problem hiding this comment.
suggestion (non-blocking): I believe this can be compressed into a single if-else statement + a helper.
| "Charge at 1 A until 4.1 V", | ||
| "Hold at 4.1 V until C/2", | ||
| "Discharge at 2 W for 1 hour", | ||
| "Discharge at C/10 for 1 hour", |
There was a problem hiding this comment.
question: was there an issue with performance or coverage on the previous experiment?
2 participants
Description
Replaces
CasadiAlgebraicSolveras the default nonlinear solver. This fixes an issue where the algebraic solver could terminate early when hitting step change tolerances but not the desired absolute tolerance. The newNonlinearSolveris tightly integrated with sundials to satisfy the same error tolerance and avoid expensive calls toIDACalcICwhere only analgebraic sub-solver is necessaryType 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