Issue 853 casadi safe

[rtimms]

Description

Changes the behaviour of casadi "safe" mode. Instead of creating a new integrator for each dt in t_eval (which scales badly with the number of points in t_eval), it now integrates in larger "global" timesteps and checks if an event has been triggered. If an event is triggered in some interval [t, t+dt] then the earliest event time is found by a root finding algorithm and the state at that time is found by interpolation. The solution is then truncated and returned up to the event time.

Fixes #853

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.

• New feature (non-breaking change which adds functionality)
• Optimization (back-end change that speeds up the code)
• Bug fix (non-breaking change which fixes an issue)

Key checklist:

• No style issues: $ flake8
• All tests pass: $ python run-tests.py --unit
• The documentation builds: $ cd docs and then $ make clean; make html

You can run all three at once, using $ python run-tests.py --quick.

Further checks:

• Code is commented, particularly in hard-to-understand areas
• Tests added that prove fix is effective or that feature works

[codecov]

Codecov Report

Merging #956 into develop will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop     #956      +/-   ##
===========================================
+ Coverage    97.34%   97.36%   +0.02%     
===========================================
  Files          222      222              
  Lines        11622    11689      +67     
===========================================
+ Hits         11313    11381      +68     
+ Misses         309      308       -1     

Impacted Files	Coverage Δ
...dels/submodels/current_collector/potential_pair.py	100.00% <ø> (ø)
pybamm/solvers/base_solver.py	100.00% <ø> (+0.27%)	⬆️
pybamm/solvers/casadi_solver.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cde479d...33aff3a. Read the comment docs.

[rtimms]

question: at the moment dt_max is set somewhat arbitrarily. should we allow this to be passed as an option so that users have control over the “global” step?

[valentinsulzer]

For dt_max, I think the most consistent thing would be to make it 1 (or 1/2 or 1/4), and assume that the user has chosen a good timescale, but also offer it as an option. (but for battery models the timescale doesn't change with current so it's not failsafe ...)

Can you leave old safe mode as an option (e.g. "old safe")? I think the sensitivity formulation will be closest to that

I also couldn't find the test where you don't suppress stderr - I wanted to see "regularity check": False or "show_eval_warnings": False can do that for you (annoyingly, there's a different options dictionary for when you create the solver and when you call it, and I don't know which you should pass to)

[valentinsulzer]

Looks great now, thanks @rtimms !

[valentinsulzer]

why did you remove this?

[rtimms]

the same check is performed on line 589

[rtimms]

thanks, just need to fix a problem where sometimes casadi gets stuck doing Linear solve failed psetup failed forever when you step too far. It doesn’t get caught in the try/except, so I need to find the right option the catches that, but doesn’t cause it to bail out early in other cases.

[valentinsulzer]

Maybe some kind of timeout? https://medium.com/@chaoren/how-to-timeout-in-python-726002bf2291