Validate the sto limits subbed into the OCPs give the correct voltage limits

[ikorotkin]

Added validator to check that the STO limits give the correct voltage limits according to issue #8 and added corresponding tests.
This will only work if the OCPs are defined as functions. It does not validate the voltage limits if the OCPs are tables.

Note that this check will likely fail the NMC parametrisation provided by A:E because of not very accurate upper/lower cut-off voltages in their example. So we either need to change the validation formula in schema.py or ask A:E to adjust their min/max sto or min/max voltages for NMC and update the repo.

I also added the target SOC check in the get_electrode_concentrations() function with tests.

This PR is not related to the BPX roadmap directly, I just wanted to start with something easier to understand how the schema works and can be validated.

[ikorotkin]

Checked the A:E parametrisation again,
LFP fails with

The minimum voltage computed from the STO limits is lower than the minimum allowed voltage

NMC fails with

The maximum voltage computed from the STO limits is higher than the maximum allowed voltage

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (develop@a88ac51). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files

@@            Coverage Diff             @@
##             develop      #32   +/-   ##
==========================================
  Coverage           ?   97.77%           
==========================================
  Files              ?        9           
  Lines              ?      314           
  Branches           ?        0           
==========================================
  Hits               ?      307           
  Misses             ?        7           
  Partials           ?        0           

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

[ejfdickinson]

I think that rather than checking higher/lower, we should check the voltage difference between computed and expected, and compare it to a given tolerance.

For example, for the LFP cell, the SOC = 0% voltage evaluates as 1.9999895288809855 - yes, this is < 2.0 but it is == 2.0 when rounded to 5 s.f. Unless we quote function coefficients and stoichiometries to a meaningless number of s.f., this type of test will often fail. However, for the same cell, the SOC = 100% voltage evaluates as 3.64856... which is appreciably wrong (> 0.1 mV error) so we probably should warn or error under such a condition.

I'll look into why we are missing by so much on the upper cut-off!

[ejfdickinson]

Separate comment: I don't think we should force target_soc to be bounded between 0 and 1 in get_electrode_concentrations().

This is because SOC is defined here by equilibration at manufacturer's cut-off voltages at a reference temperature. It is perfectly possible for the cell to have a state outside these limits - for example, if it is brought to equilibrium at 4.2 V at a higher/lower temperature, and then the temperature is changed and it is allowed to equilibrate at the reference temperature, it might have a voltage > 4.2 V and therefore SOC > 1.

The actual unphysical states are those for which xLi < 0 or xLi > 1. These should probably provoke a user's choice of warning or error.

[rtimms]

Agree we should add use some tolerance for these checks (ideally the user can control this tolerance when they create the BPX object) and maybe raise a warning instead of an error.

Also agree on allowing initial SOC to be outside of [0,1].

Otherwise looks good to go, thanks!

[ikorotkin]

Agreed on using some user-defined tolerance, a warning instead of an error, and the initial SoC to be outside [0,1].

What do you think would be the best way of defining the tolerance? Or how a user would pass the tolerance to the BPX object?

EDIT:

From @rtimms email:

For the tolerances you could either have some settings configuration (bpx.settings.blah = ...) or pass extra options to when you create the BPX object.

[ikorotkin]

• Make it compatible with the recent changes in develop
• Allow initial SOC to be outside of [0,1]
• Add tolerance for checking the cut-off voltage difference between computed and expected
• Update tests

[codecov-commenter]

Codecov Report

All modified lines are covered by tests ✅

❗ No coverage uploaded for pull request base (develop@79ecbc2). Click here to learn what that means.

Additional details and impacted files

@@            Coverage Diff             @@
##             develop      #32   +/-   ##
==========================================
  Coverage           ?   96.81%           
==========================================
  Files              ?       11           
  Lines              ?      440           
  Branches           ?        0           
==========================================
  Hits               ?      426           
  Misses             ?       14           
  Partials           ?        0           

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

[ikorotkin]

I updated this PR @rtimms, @ejfdickinson.

Allowed the initial SOC to be outside of [0,1], added user-defined tolerance, changed errors to warnings, and added tests.

By default, the tolerance is 1 mV. This can give a warning, for example:

import bpx

p=bpx.parse_bpx_file("nmc_pouch_cell_BPX.json")

/home/ik3g18/workspace/BPX/bpx/validators.py:32: UserWarning: The maximum voltage computed from the STO limits (4.201761488607647 V) is higher than the upper voltage cut-off (4.2 V) with the absolute tolerance v_tol = 0.001 V
  warn(

The tolerance can be defined as:

q=bpx.parse_bpx_file("nmc_pouch_cell_BPX.json", v_tol=0.005)

or

BPX.settings.v_tol = 0.005

Let me know if I need to change anything (including variable names, warning messages, etc.)

[rtimms]

we should also add v_tol to parse_bpx_obj and parse_bpx_str

[ikorotkin]

This is a bit tricky because if I overwrite parse_obj in ExtraBaseModel class, I'll break other methods that call parse_obj.

What would be the most natural and 'future-proof' way of introducing parameter v_tol here?

[rtimms]

We could use have some global settings, e.g. have a file

class Settings(object):
    tolerances = {
        "Voltage [V]": 1e-3,  
    }

settings = Settings()

then expose settings as part of bpx so we can do bpx.settings.tolerances["Voltage [V]"] = v_tol to update the tolerance before reading a BPX? Maybe there is a cleaner solution.

[ikorotkin]

Good idea, thanks!

I changed this setting to bpx.settings.tolerances["Voltage [V]"] but left it inside ExtraBaseModel in schema.py for now.
It's just one parameter at the moment.

[ikorotkin]

Now parse_bpx_obj and parse_bpx_str have optional v_tol parameter as well. Added a few more tests to check that it actually works.

[rtimms]

thanks!