Faster (re)initialization of ODEs in IDA

[MarcBerliner]

Description

In IDA, we can compute an analytical solution for consistent initialization/reinitialization of ODEs, which avoids an expensive call to IDACalcIC. This change gives a modest speed improvement to all the ODE models solved by the IDAKLUSolver and will give a bigger speedup to those with a dense t_eval vector. For example,

import pybamm
import numpy as np

model = pybamm.lithium_ion.SPM()
solver = pybamm.IDAKLUSolver()
sim = pybamm.Simulation(model, solver=solver)

t_eval = np.linspace(0, 3600, 10000)
sol = sim.solve(t_eval)

print(sol.integration_time)

• New integration_time: 367 ms
• Old integration_time: 514 ms
• Speedup: 1.4x

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.

• Optimization (back-end change that speeds up the code)

Key checklist:

• No style issues: $ pre-commit run (or $ nox -s pre-commit) (see CONTRIBUTING.md for how to set this up to run automatically when committing locally, in just two lines of code)
• All tests pass: $ python run-tests.py --all (or $ nox -s tests)
• The documentation builds: $ python run-tests.py --doctest (or $ nox -s doctests)

You can run integration tests, unit tests, and doctests together at once, using $ python run-tests.py --quick (or $ nox -s 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

All modified and coverable lines are covered by tests ✅

Project coverage is 99.46%. Comparing base (5bb146f) to head (75ca46b).
Report is 2 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #4453   +/-   ##
========================================
  Coverage    99.46%   99.46%           
========================================
  Files          293      293           
  Lines        22381    22381           
========================================
  Hits         22261    22261           
  Misses         120      120           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[kratman]

If id_i is a bool, then the id_i == 1 should be redundant

[martinjrobins]

This bit is a confusing jumble of types, these are not really bool, but are floats indicating if the state is differential (1.0) or algebraic (0.0), this is just the format that sundials expects. I think introducing a bool here is just confusing an already confusing sundials thing (e.g. they never say in the docs what happens to values that are not 0 or 1!), perhaps:

id_val[ii] = id_np_val[ii];
is_ODE &= id_val[ii] == 1.0

[kratman]

Float comparisons are not reliable either. Float equality checks are not good to have in the code

[MarcBerliner]

@martinjrobins good point! I'll make this change

[kratman]

@martinjrobins To avoid a float equality comparison we should have something like

is_ODE &= id_val[ii] > SMALL_NUMBER

That way we are not expecting equality and we just check that it is greater than zero.

[MarcBerliner]

I made this an inequalty check to handle minor floating point arithmetic issues

[martinjrobins]

@kratman: totally agree! but this is how sundials docs define this vector (as a vector of floats, 1.0 = differential and 0.0 == algebraic) so I think we have to follow this definition, no matter how strange it is. Also, I'm not a fan of implicit type conversion, which is happening here twice (once from realtype -> bool, then from bool -> realtype)

[martinjrobins]

saying that, its not a big deal so happy with is_ODE &= id_val[ii] > SMALL_NUMBER, as long as the type conversions are removed. it shouldn't make any difference to the result

[kratman]

Yeah sorry for the confusion, I was talking to Marc separately removing the type conversion was already part of it

[martinjrobins]

looks great @MarcBerliner, just that one minor comment about the bool. I'm supprised that sundials doesn't do this in CalcIC, it has the id vector so can tell if it is an ode or not. Great find!