Idaklu solver can be given a list of variables to calculate during the solve

[jsbrittain]

Description

Provides a mode for the (Idaklu) solver so that only a user given list of variables are calculated and stored as part of the solution. This functionality extends to sensitivity calculations. The PR also includes a restructuring of the idaklu c++ codebase to support further extensions and abstractions in the future.

Fixes #2796

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: $ pre-commit run (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
• The documentation builds: $ python run-tests.py --doctest

You can run unit and doctests together 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

[jsbrittain]

Interface is as suggested in #2796 , namely:

solver = pybamm.IDAKLUSolver()
sol = solver.solve(model, t_eval, output_variables=['Voltage [V]'])
print(sol['Voltage [V]'].entries)

Note that only variables listed in output_variables can be queried since the full model state vector is no longer propagated and returned.

[codecov]

Codecov Report

All modified lines are covered by tests ✅

Comparison is base (acad5fa) 99.56% compared to head (9ac8383) 99.58%.

❗ Current head 9ac8383 differs from pull request most recent head c7d109b. Consider uploading reports for the commit c7d109b to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3217      +/-   ##
===========================================
+ Coverage    99.56%   99.58%   +0.01%     
===========================================
  Files          253      254       +1     
  Lines        19559    19811     +252     
===========================================
+ Hits         19474    19728     +254     
+ Misses          85       83       -2     

Files	Coverage Δ
pybamm/__init__.py	100.00% <100.00%> (ø)
pybamm/solvers/base_solver.py	100.00% <100.00%> (ø)
pybamm/solvers/idaklu_solver.py	100.00% <100.00%> (+0.90%)	⬆️
pybamm/solvers/processed_variable.py	100.00% <100.00%> (ø)
pybamm/solvers/processed_variable_computed.py	100.00% <100.00%> (ø)
pybamm/solvers/solution.py	100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

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

[martinjrobins]

Thanks @jsbrittain. The doctests and notebooks, and the example scripts don't seem anything to do with this PR, but there are a few issues to fix in the Cadacy Static Code Analysis

[jsbrittain]

Thanks @jsbrittain. The doctests and notebooks, and the example scripts don't seem anything to do with this PR, but there are a few issues to fix in the Cadacy Static Code Analysis

@martinjrobins yes, I believe the doctest and notebooks are due to matplotlib deprecating a feature over the weekend - there are other PRs in progress to resolve this. As for codacy, I was looking at the report and the (big!) jump in codacy issues appears to be due to the macro function that was introduced in CasadiSolverOpenMP_solvers.hpp to help automate creation of classes for the various solvers. Codacy doesn't appear to expand the expressions when evaluating them and so believes that there are now many dangling properties. The only way I can think of adequately resolving this would be to revert the change and remove the macro. Do you have a strong preference here?

[brosaplanella]

@martinjrobins yes, I believe the doctest and notebooks are due to matplotlib deprecating a feature over the weekend - there are other PRs in progress to resolve this.

The fixes are now merged in the develop branch

[martinjrobins]

. The only way I can think of adequately resolving this would be to revert the change and remove the macro. Do you have a strong preference here?

I would be in favor of keeping the macro, can you turn off the error in some way?

[brosaplanella]

Seems like Codacy got stuck. Is it an issue at their end or at our end?

[jsbrittain]

Seems like Codacy got stuck. Is it an issue at their end or at our end?

@brosaplanella Not sure why codacy stalled. I've been making some changes and commited again so will keep an eye on it.

[jsbrittain]

@martinjrobins Two main changes since the last review:

1. I've replaced the macro with a variadic arguments approach. This keeps the boilerplate down, but should also make the code easier for linters to follow and to debug going forwards.
2. Despite the above, codacy still has trouble keeping track of some of the parent class variables passed in this way, so I've added cppcheck-suppress statements on these lines (since this is what codacy [currently] uses) and provided comments in the code to justify them.

[martinjrobins]

fantastic, thanks @jsbrittain. All looks good to merge