-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Validate the sto limits subbed into the OCPs give the correct voltage limits #32
Merged
rtimms
merged 19 commits into
FaradayInstitution:develop
from
ikorotkin:issue-8-sto-limits
Oct 19, 2023
Merged
Changes from all commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
7411768
Validate the sto limits according to issue #8
ikorotkin e836bbe
Fixed tests, added target_soc tests
ikorotkin 233f4db
Fix too big maximum concentration in the example
ikorotkin 6ca9e7b
Added tests for the STO limit validator
ikorotkin 1339711
Improved error messages for the STO limits validator
ikorotkin 5eeb7e9
Add skip_on_failure for the sto limit validator (no need to check the…
ikorotkin b46ddc4
Merged development into issue-8-sto-limits
ikorotkin 14f2c11
Added STO limit check for all parameter sets (including SPM)
ikorotkin b9200d1
Changed error to a warning if initial SOC is outside of [0,1]
ikorotkin 7a54d44
Changed error to a warning for STO limits validator
ikorotkin 19d3dcf
Moved STO validator from schema to new file validators.py
ikorotkin 755ccfd
Added user-defined tolerance to check minimum/maximum voltage limits.…
ikorotkin d18380d
Treat more warnings as errors for testing
ikorotkin 3524977
Updated CHANGELOG
ikorotkin e4168ef
Merge develop
ikorotkin 337624b
Merge develop
ikorotkin 1d51b3d
Add v_tol to parse_bpx_obj and parse_bpx_str, rename the global setti…
ikorotkin cd577b9
Added tests
ikorotkin df537c7
Split long strings into several lines
ikorotkin File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,49 +1,70 @@ | ||
from bpx import BPX | ||
|
||
|
||
def parse_bpx_file(filename: str) -> BPX: | ||
def parse_bpx_file(filename: str, v_tol: float = 0.001) -> BPX: | ||
""" | ||
Parameters | ||
---------- | ||
|
||
filename: str | ||
a filepath to a bpx file | ||
v_tol: float | ||
absolute tolerance in [V] to validate the voltage limits, 1 mV by default | ||
|
||
Returns | ||
------- | ||
BPX: | ||
a parsed BPX model | ||
""" | ||
if v_tol < 0: | ||
raise ValueError("v_tol should not be negative") | ||
|
||
BPX.settings.tolerances["Voltage [V]"] = v_tol | ||
|
||
return BPX.parse_file(filename) | ||
|
||
|
||
def parse_bpx_obj(bpx: dict) -> BPX: | ||
def parse_bpx_obj(bpx: dict, v_tol: float = 0.001) -> BPX: | ||
""" | ||
Parameters | ||
---------- | ||
|
||
bpx: dict | ||
a dict object in bpx format | ||
v_tol: float | ||
absolute tolerance in [V] to validate the voltage limits, 1 mV by default | ||
|
||
Returns | ||
------- | ||
BPX: | ||
a parsed BPX model | ||
""" | ||
if v_tol < 0: | ||
raise ValueError("v_tol should not be negative") | ||
|
||
BPX.settings.tolerances["Voltage [V]"] = v_tol | ||
|
||
return BPX.parse_obj(bpx) | ||
|
||
|
||
def parse_bpx_str(bpx: str) -> BPX: | ||
def parse_bpx_str(bpx: str, v_tol: float = 0.001) -> BPX: | ||
""" | ||
Parameters | ||
---------- | ||
|
||
bpx: str | ||
a json formatted string in bpx format | ||
v_tol: float | ||
absolute tolerance in [V] to validate the voltage limits, 1 mV by default | ||
|
||
Returns | ||
------- | ||
BPX: | ||
a parsed BPX model | ||
""" | ||
if v_tol < 0: | ||
raise ValueError("v_tol should not be negative") | ||
|
||
BPX.settings.tolerances["Voltage [V]"] = v_tol | ||
|
||
return BPX.parse_raw(bpx) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,47 @@ | ||
from warnings import warn | ||
|
||
|
||
def check_sto_limits(cls, values): | ||
""" | ||
Validates that the STO limits subbed into the OCPs give the correct voltage limits. | ||
Works if both OCPs are defined as functions. | ||
Blended electrodes are not supported. | ||
This is a reusable validator to be used for both DFN/SPMe and SPM parameter sets. | ||
""" | ||
|
||
try: | ||
ocp_n = values.get("negative_electrode").ocp.to_python_function() | ||
ocp_p = values.get("positive_electrode").ocp.to_python_function() | ||
except AttributeError: | ||
# OCPs defined as interpolated tables or one of the electrodes is blended; do nothing | ||
return values | ||
|
||
sto_n_min = values.get("negative_electrode").minimum_stoichiometry | ||
sto_n_max = values.get("negative_electrode").maximum_stoichiometry | ||
sto_p_min = values.get("positive_electrode").minimum_stoichiometry | ||
sto_p_max = values.get("positive_electrode").maximum_stoichiometry | ||
V_min = values.get("cell").lower_voltage_cutoff | ||
V_max = values.get("cell").upper_voltage_cutoff | ||
|
||
# Voltage tolerance from `settings` data class | ||
tol = cls.settings.tolerances["Voltage [V]"] | ||
|
||
# Checks the maximum voltage estimated from STO | ||
V_max_sto = ocp_p(sto_p_min) - ocp_n(sto_n_max) | ||
if V_max_sto - V_max > tol: | ||
warn( | ||
f"The maximum voltage computed from the STO limits ({V_max_sto} V) " | ||
f"is higher than the upper voltage cut-off ({V_max} V) " | ||
f"with the absolute tolerance v_tol = {tol} V" | ||
) | ||
|
||
# Checks the minimum voltage estimated from STO | ||
V_min_sto = ocp_p(sto_p_max) - ocp_n(sto_n_min) | ||
if V_min_sto - V_min < -tol: | ||
warn( | ||
f"The minimum voltage computed from the STO limits ({V_min_sto} V) " | ||
f"is less than the lower voltage cut-off ({V_min} V) " | ||
f"with the absolute tolerance v_tol = {tol} V" | ||
) | ||
|
||
return values |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,123 @@ | ||
import unittest | ||
import warnings | ||
import copy | ||
|
||
from bpx import BPX, parse_bpx_file, parse_bpx_obj, parse_bpx_str | ||
|
||
|
||
class TestParsers(unittest.TestCase): | ||
def setUp(self): | ||
base = """ | ||
{ | ||
"Header": { | ||
"BPX": 0.1, | ||
"Title": "Parameterisation example of an NMC111|graphite 12.5 Ah pouch cell", | ||
"Model": "DFN" | ||
}, | ||
"Parameterisation": { | ||
"Cell": { | ||
"Ambient temperature [K]": 298.15, | ||
"Initial temperature [K]": 298.15, | ||
"Reference temperature [K]": 298.15, | ||
"Lower voltage cut-off [V]": 2.7, | ||
"Upper voltage cut-off [V]": 4.2, | ||
"Nominal cell capacity [A.h]": 12.5, | ||
"Specific heat capacity [J.K-1.kg-1]": 913, | ||
"Thermal conductivity [W.m-1.K-1]": 2.04, | ||
"Density [kg.m-3]": 1847, | ||
"Electrode area [m2]": 0.016808, | ||
"Number of electrode pairs connected in parallel to make a cell": 34, | ||
"External surface area [m2]": 0.0379, | ||
"Volume [m3]": 0.000128 | ||
}, | ||
"Electrolyte": { | ||
"Initial concentration [mol.m-3]": 1000, | ||
"Cation transference number": 0.2594, | ||
"Conductivity [S.m-1]": "0.1297 * (x / 1000) ** 3 - 2.51 * (x / 1000) ** 1.5 + 3.329 * (x / 1000)", | ||
"Diffusivity [m2.s-1]": "8.794e-11 * (x / 1000) ** 2 - 3.972e-10 * (x / 1000) + 4.862e-10", | ||
"Conductivity activation energy [J.mol-1]": 17100, | ||
"Diffusivity activation energy [J.mol-1]": 17100 | ||
}, | ||
"Negative electrode": { | ||
"Particle radius [m]": 4.12e-06, | ||
"Thickness [m]": 5.62e-05, | ||
"Diffusivity [m2.s-1]": 2.728e-14, | ||
"OCP [V]": | ||
"9.47057878e-01 * exp(-1.59418743e+02 * x) - 3.50928033e+04 + | ||
1.64230269e-01 * tanh(-4.55509094e+01 * (x - 3.24116012e-02 )) + | ||
3.69968491e-02 * tanh(-1.96718868e+01 * (x - 1.68334476e-01)) + | ||
1.91517003e+04 * tanh(3.19648312e+00 * (x - 1.85139824e+00)) + | ||
5.42448511e+04 * tanh(-3.19009848e+00 * (x - 2.01660395e+00))", | ||
"Entropic change coefficient [V.K-1]": | ||
"(-0.1112 * x + 0.02914 + 0.3561 * exp(-((x - 0.08309) ** 2) / 0.004616)) / 1000", | ||
"Conductivity [S.m-1]": 0.222, | ||
"Surface area per unit volume [m-1]": 499522, | ||
"Porosity": 0.253991, | ||
"Transport efficiency": 0.128, | ||
"Reaction rate constant [mol.m-2.s-1]": 5.199e-06, | ||
"Minimum stoichiometry": 0.005504, | ||
"Maximum stoichiometry": 0.75668, | ||
"Maximum concentration [mol.m-3]": 29730, | ||
"Diffusivity activation energy [J.mol-1]": 30000, | ||
"Reaction rate constant activation energy [J.mol-1]": 55000 | ||
}, | ||
"Positive electrode": { | ||
"Particle radius [m]": 4.6e-06, | ||
"Thickness [m]": 5.23e-05, | ||
"Diffusivity [m2.s-1]": 3.2e-14, | ||
"OCP [V]": | ||
"-3.04420906 * x + 10.04892207 - 0.65637536 * tanh(-4.02134095 * (x - 0.80063948)) + | ||
4.24678547 * tanh(12.17805062 * (x - 7.57659337)) - 0.3757068 * tanh(59.33067782 * (x - 0.99784492))", | ||
"Entropic change coefficient [V.K-1]": -1e-4, | ||
"Conductivity [S.m-1]": 0.789, | ||
"Surface area per unit volume [m-1]": 432072, | ||
"Porosity": 0.277493, | ||
"Transport efficiency": 0.1462, | ||
"Reaction rate constant [mol.m-2.s-1]": 2.305e-05, | ||
"Minimum stoichiometry": 0.42424, | ||
"Maximum stoichiometry": 0.96210, | ||
"Maximum concentration [mol.m-3]": 46200, | ||
"Diffusivity activation energy [J.mol-1]": 15000, | ||
"Reaction rate constant activation energy [J.mol-1]": 35000 | ||
}, | ||
"Separator": { | ||
"Thickness [m]": 2e-05, | ||
"Porosity": 0.47, | ||
"Transport efficiency": 0.3222 | ||
} | ||
} | ||
} | ||
""" | ||
self.base = base.replace("\n", "") | ||
|
||
def test_negative_v_tol_file(self): | ||
with self.assertRaisesRegex( | ||
ValueError, | ||
"v_tol should not be negative", | ||
): | ||
parse_bpx_file("filename", v_tol=-0.001) | ||
|
||
def test_negative_v_tol_object(self): | ||
bpx_obj = {"BPX": 1.0} | ||
with self.assertRaisesRegex( | ||
ValueError, | ||
"v_tol should not be negative", | ||
): | ||
parse_bpx_obj(bpx_obj, v_tol=-0.001) | ||
|
||
def test_negative_v_tol_string(self): | ||
with self.assertRaisesRegex( | ||
ValueError, | ||
"v_tol should not be negative", | ||
): | ||
parse_bpx_str("String", v_tol=-0.001) | ||
|
||
def test_parse_string(self): | ||
test = copy.copy(self.base) | ||
with self.assertWarns(UserWarning): | ||
parse_bpx_str(test) | ||
|
||
def test_parse_string_tolerance(self): | ||
warnings.filterwarnings("error") # Treat warnings as errors | ||
test = copy.copy(self.base) | ||
parse_bpx_str(test, v_tol=0.002) |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should also add
v_tol
toparse_bpx_obj
andparse_bpx_str
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a bit tricky because if I overwrite
parse_obj
inExtraBaseModel
class, I'll break other methods that callparse_obj
.What would be the most natural and 'future-proof' way of introducing parameter
v_tol
here?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could use have some global settings, e.g. have a file
then expose
settings
as part ofbpx
so we can dobpx.settings.tolerances["Voltage [V]"] = v_tol
to update the tolerance before reading a BPX? Maybe there is a cleaner solution.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea, thanks!
I changed this setting to
bpx.settings.tolerances["Voltage [V]"]
but left it insideExtraBaseModel
inschema.py
for now.It's just one parameter at the moment.