I2217 idaklu

[martinjrobins]

Description

Work related to #2217. Added an options argument to the idaklu solver:

    options: dict, optional
        Addititional options to pass to the solver, by default:
        {
            print_stats: False, # print statistics of the solver after every solve
            jacobian: "sparse", # jacobian form, can be "none", "dense", "sparse",
                                # "matrix-free"
            linear_solver: "SUNLinSol_KLU", # name of sundials linear solver to use
                                  # options are: "SUNLinSol_KLU",
                                  # "SUNLinSol_Dense", "SUNLinSol_LapackDense"
                                  # "SUNLinSol_SPBCGS", "SUNLinSol_SPFGMR",
                                  # "SUNLinSol_SPGMR", "SUNLinSol_SPTFQMR",
            preconditioner: "BBDP", # preconditioner for iterative solvers,
                                    # can be "none", "BBDP"
            linsol_max_iterations: 5, # for iterative linear solvers, max number of
                                      # iterations
            precon_half_bandwidth: 5, # for iterative linear solver preconditioner,
                                      # bandwidth of approximate jacobian
            precon_half_bandwidth_keep: 5 #for iterative linear solver preconditioner,
                                      # bandwidth of approximate jacobian that is kept

        }
        Note: These options only have an effect if model.convert_to_format == 'casadi'

The iterative solvers only seem to work well for SPM and SPMe models, my best guess atm is that the preconditioner I'm using is not suitable for DFN. Sundials unfortunately only has a single preconditioner (BBDP), but we could manually implement a preconditioner if anyone knows a good one for DFN

I've also built in support for sundials v5 and v6, as the api changed in v6

I've also generally restructured the idaklu C++ code (the casadi stuff, we are going to drop the python idaklu solver...) to be a bit nicer

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)

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

[valentinsulzer]

we are going to drop the python idaklu solver

What does this mean? How will we interface to the C++ code?

[martinjrobins]

we are going to drop the python idaklu solver

What does this mean? How will we interface to the C++ code?

Sorry, this statement wasn't very clear. We have two C++ solvers, one which is used for model.convert_to_format == 'casadi', and another (the original) that is used when model.convert_to_format == 'python'. The plan is to keep the former, and drop the latter. Rather than maintain two separate C++ solvers.

[codecov]

Codecov Report

Base: 99.72% // Head: 99.72% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (eed75ab) compared to base (d5547da).
Patch coverage: 100.00% of modified lines in pull request are covered.

❗ Current head eed75ab differs from pull request most recent head 2b80d6b. Consider uploading reports for the commit 2b80d6b to get more accurate results

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #2444   +/-   ##
========================================
  Coverage    99.72%   99.72%           
========================================
  Files          258      258           
  Lines        19184    19200   +16     
========================================
+ Hits         19131    19147   +16     
  Misses          53       53           

Impacted Files	Coverage Δ
pybamm/solvers/idaklu_solver.py	99.06% <100.00%> (+0.07%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[rtimms]

thanks @martinjrobins looks good! should we do some checking of the options since it seems sundials doesn't raise and error if you pass a bad option for the jacobian or lin solver?

[martinjrobins]

Running the failed benchmark (time to setup DFN for ORegan2022 param) on my machine shows no difference between develop and this branch. @rtimms @tinosulzer are you happy if I treat this as a false positive and merge this PR?