Issue 664 casadi #687
Issue 664 casadi #687
Conversation
Codecov Report
@@ Coverage Diff @@
## master #687 +/- ##
=========================================
+ Coverage 98.48% 98.5% +0.02%
=========================================
Files 171 173 +2
Lines 8589 8776 +187
=========================================
+ Hits 8459 8645 +186
- Misses 130 131 +1
Continue to review full report at Codecov.
|
pybamm/solvers/casadi_solver.py
Outdated
| solve_time = timer.time() - solve_start_time | ||
|
|
||
| # Identify the event that caused termination | ||
| termination = self.get_termination_reason(solution, self.events) |
There was a problem hiding this comment.
You managed to get events working?
There was a problem hiding this comment.
Not yet, will remove this
| return states_eval - model.mass_matrix.entries @ ydot | ||
|
|
||
| # Create event-dependent function to evaluate events | ||
| def event_fun(event): |
There was a problem hiding this comment.
If events aren't working should we remove this bit of code?
There was a problem hiding this comment.
Still need this for setting up with casadi and passing to other solvers
| @@ -53,7 +53,7 @@ def rhs(t, y): | |||
| def alg(t, y): | |||
| return np.array([1 - y[1]]) | |||
|
|
|||
| solver = pybamm.IDAKLU() | |||
| solver = pybamm.IDAKLUSolver() | |||
There was a problem hiding this comment.
Not a fan of my naming haha
There was a problem hiding this comment.
looks great thanks @tinosulzer ! might be good to update the install instructions to say that pybamm comes with casadi so can solve daes, but doesn't support events at the moment. then recommend installing either scikits or KLU (probably recommend KLU since it performs better for large systems). then the line in the scikits install about needing sundials for DFN can be removed as you only need to install sundials separately if you want to use the optional scikits ot KLU
| @@ -96,7 +103,7 @@ def __init__(self, name="Unnamed model"): | |||
| # Default behaviour is to use the jacobian and simplify | |||
| self.use_jacobian = True | |||
| self.use_simplify = True | |||
| self.use_to_python = True | |||
| self.convert_to_format = "python" | |||
There was a problem hiding this comment.
if casadi performs better should we just use that? though maybe nice to keep the pybamm expression tree by default. maybe it would be good to add a notebook or some kind of readme about using pybamm with casadi to highlight why/when you might want to use this option
There was a problem hiding this comment.
Will update the install instructions separately in #646 as I've made a few changes there anyway. Casadi is faster than python but harder to debug (not sure how you step through the expression tree to see which bit of code is messing up) so not sure which one should be default? I lean towards casadi
There was a problem hiding this comment.
Actually, casadi doesn't handle interpolation yet (in pybamm, it has the functionality but needs to be wrapped), and other similar issues might appear, so I will leave python as default until more confident with casadi
Description
Use CasADi to provide big speed-up to both setup and solve:
Fixes #664
Still to do:
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.
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: