Make basic models compatible with experiments

[rtimms]

Description

Makes "basic" models compatible with experiments. For a model to work with experiments it needs to have

1. All the Variable objects in the variables dict
2. The "Current function [A]" and "Battery voltage [V]" variables
   3. Define self.summary_variables

Related #3908

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 (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

Attention: Patch coverage is 98.27586% with 1 lines in your changes are missing coverage. Please review.

Project coverage is 99.58%. Comparing base (448450b) to head (f214f3c).
Report is 3 commits behind head on develop.

Files	Patch %	Lines
..._battery_models/lithium_ion/basic_dfn_composite.py	75.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3995      +/-   ##
===========================================
- Coverage    99.59%   99.58%   -0.01%     
===========================================
  Files          259      260       +1     
  Lines        21353    21358       +5     
===========================================
+ Hits         21266    21270       +4     
- Misses          87       88       +1     

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

[valentinsulzer]

These changes are a step in the right direction, so ok to merge as is if you don't have time to work on it further, but in the longer term it would be better to change how summary variables are processed in Solution so that basic models work out of the box without needing to add all the right variables.

Easy fixes:

• make the default summary variables for a base lithium ion model be [], and only set the specific summary variables for DFN, SPM, SPMe, etc
• don't calculate summary variables based on Discharge capacity and Battery voltage if those are not included in self.summary_variables

Harder fixes that also address the fact that summary variable calculations can slow down the simulation quite a lot:

• create a SummaryVariable object that saves solution.last_state and uses it to calculate the esoh variables only when they are called (the same way as Solution calculates the variables only when they are called)
• get rid of the min/max voltage and capacity calculations as a default and allow users to pass in functions that calculate summary variables based on other variables

[rtimms]

Most of the new variables were added so that copying from one model to the next in experiments worked (you need to Variable objects in the variables dict).

make the default summary variables for a base lithium ion model be [], and only set the specific summary variables for DFN, SPM, SPMe, etc

where would you put these to avoid code duplication?

don't calculate summary variables based on Discharge capacity and Battery voltage if those are not included in self.summary_variables

I can do this. Edit: "Battery voltage [V]" has to be in variables for experiments to work and isn't in summary_variables.

I'll leave the harder fixes for a separate issue.

[valentinsulzer]

where would you put these to avoid code duplication?

rename lithium_ion.BaseModel.set_summary_variables to set_standard_summary_variables and only call it from within DFN.__init__, SPM.__init__, etc (instead of BaseModel.__init__).

"Battery voltage [V]" has to be in variables for experiments to work and isn't in summary_variables

Let's still only calculate the related min/max voltage if it's there (not there by default but possible to add) i.e. change

if "Battery voltage [V]" in model.variables

to

if "Min/max battery voltage [V]" in model.summary_variables

and same for "Discharge energy". Calculating the min/max voltage for every step requires evaluating the full voltage, which is much slower than solving the model for SPM/SPMe.

[rtimms]

Ah yeah, thanks, obvious implementation now!

For the harder issues, I think we should should store all_last_states in the same way we store all_first_states, then we can just lazily evaluate summary variables. The only catch is you won’t be able to do things like min/max voltage as you may not have the whole cycle solution (you may not have saved that cycle). I think for summary variables we should make it so they can only depend on the first and last state of a cycle, so you can have things that are (some combination of) variables at the start/end of a cycle. Then you can still do “Change in…” summary variables.

For custom summary variables I wonder if it’s simpler to just add new variables to the model rather than allowing functions to be passed to summary variables? Then summary variables can still just be a list.

If this sounds like a sensible approach I’ll do it as part of this PR.

[rtimms]

Alternatively we can calculate the “easy” summary variables on the fly and do the above for the esoh. Is that what you had in mind?

I’m leaning towards just allowing summary variables to depend on first and last state, and if people want something else then they should save that cycle or do some other manual calculation.

[valentinsulzer]

Yeah I think it's reasonable to only have summary variables based on first/last state and have users do something manually if they want something more complicated.

For custom summary variables I wonder if it’s simpler to just add new variables to the model rather than allowing functions to be passed to summary variables? Then summary variables can still just be a list.

I was talking about custom functions that allow you to do things over different time steps (min/max, integrate, etc), which wouldn't be possible to define as a model variable, but let's leave that for later if there is a good use case

Alternatively we can calculate the “easy” summary variables on the fly and do the above for the esoh. Is that what you had in mind?

This is probably ok but we should profile it. There is some overhead to creating a ProcessedVariable which you then never use

[rtimms]

I was talking about custom functions that allow you to do things over different time steps (min/max, integrate, etc), which wouldn't be possible to define as a model variable, but let's leave that for later if there is a good use case

For these you wouldn't be able to do from first/last state anyway, so I say let's leave it for later.

This is probably ok but we should profile it. There is some overhead to creating a ProcessedVariable which you then never use

If we aren't doing min/max etc then let's just lazily evaluate everything when you call.

[brosaplanella]

Sounds good to me. We should keep track of what's needed to build a successful basic model so we can document it well and users can build their own models.

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[rtimms]

@valentinsulzer I would like to merge this and have made a new issue for evaluating summary variables only when they are called #4058

Since #4039 I realised that the way we set up steps in an experiment adds equations for discharge capacity and throughput capacity, even when they aren't in the original model! I don't think this is the intended behaviour, so I added an extra argument to the external circuit models to control this. I'm not super happy with that as an implementation, so happy to receive feedback. I thought about doing it via another option, but 1) we already way too many options and 2) we probably want it to be true for e.g. SPM, DFN but not for all base lithium ion models.

[rtimms]

We can't do this if we don't allow calculating summary variables on the fly. I'm not sure how useful setting a voltage cut-off at the experiment level in this way is anyway? Surely we would set cut-offs at the step level?

[valentinsulzer]

It was added in response to this request #1826. I completely forgot we had this feature but it's quite useful for GITT! The lines commented here are just for logging, the line that does the check is in the Simulation class, we should keep it but evaluate the voltage inside the Simulation class instead of using the summary variable. It would only get called if voltage_stop is not None, so no performance hit in general.

[rtimms]

We add these by default when using this as a submodel, but pass add_discharge_capacity=False when creating the submodel to update the "Current [A]" variable for experiments to avoid adding these equations to models that don't already contain them. I'd welcome alternative implementations.

[valentinsulzer]

How about just having a separate submodel for discharge capacity and related variables?

[valentinsulzer]

Main changes:

• Keep voltage termination at the experiment level (e.g. for GITT) but evaluate the voltage directly instead of using the summary variable
• Try having a separate submodel for discharge capacity (there may be a reason why this isn't possible)

[valentinsulzer]

It was added in response to this request #1826. I completely forgot we had this feature but it's quite useful for GITT! The lines commented here are just for logging, the line that does the check is in the Simulation class, we should keep it but evaluate the voltage inside the Simulation class instead of using the summary variable. It would only get called if voltage_stop is not None, so no performance hit in general.