Skip to content
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

Validation based on models #34

Conversation

ikorotkin
Copy link
Collaborator

A (working) attempt to add validation based on the model according to issue #29 and following roadmap #22.

This PR is based on #33 and supports blended electrodes for all models.

Added multiple tests and validation that the given model (SPM, SPMe, or DFN) corresponds to a proper parameter set. In the case of SPM, some of the parameters should not be included:

"Header": {
    "BPX": 1.0,
    "Model": "SPM",
},
"Parameterisation": {
    "Cell": {
        "Ambient temperature [K]": 299.0,
        "Initial temperature [K]": 299.0,
        "Reference temperature [K]": 299.0,
        "Electrode area [m2]": 2.0,
        "External surface area [m2]": 2.2,
        "Volume [m3]": 1.0,
        "Number of electrode pairs connected in parallel to make a cell": 1,
        "Nominal cell capacity [A.h]": 5.0,
        "Lower voltage cut-off [V]": 2.0,
        "Upper voltage cut-off [V]": 4.0,
    },
    "Negative electrode": {
        "Particle radius [m]": 5.86e-6,
        "Thickness [m]": 85.2e-6,
        "Diffusivity [m2.s-1]": 3.3e-14,
        "OCP [V]": {"x": [0, 0.1, 1], "y": [1.72, 1.2, 0.06]},
        "Surface area per unit volume [m-1]": 383959,
        "Reaction rate constant [mol.m-2.s-1]": 1e-10,
        "Maximum concentration [mol.m-3]": 33133,
        "Minimum stoichiometry": 0.01,
        "Maximum stoichiometry": 0.99,
    },
    "Positive electrode": {
        "Thickness [m]": 75.6e-6,
        "Particle": {
            "Primary": {
                "Particle radius [m]": 5.22e-6,
                "Diffusivity [m2.s-1]": 4.0e-15,
                "OCP [V]": {"x": [0, 0.1, 1], "y": [1.72, 1.2, 0.06]},
                "Surface area per unit volume [m-1]": 382184,
                "Reaction rate constant [mol.m-2.s-1]": 1e-10,
                "Maximum concentration [mol.m-3]": 63104.0,
                "Minimum stoichiometry": 0.1,
                "Maximum stoichiometry": 0.9,
            },
            "Secondary": {
                "Particle radius [m]": 10.0e-6,
                "Diffusivity [m2.s-1]": 4.0e-15,
                "OCP [V]": {"x": [0, 0.1, 1], "y": [1.72, 1.2, 0.06]},
                "Surface area per unit volume [m-1]": 382184,
                "Reaction rate constant [mol.m-2.s-1]": 1e-10,
                "Maximum concentration [mol.m-3]": 63104.0,
                "Minimum stoichiometry": 0.1,
                "Maximum stoichiometry": 0.9,
            },
        },
    },
},

Note if we change the Model field to DFN or SPMe in this example, this will produce an error.
Also note how we defined blended electrode (positive in this example).
Some of the parameters are optional.

@codecov
Copy link

codecov bot commented Jun 21, 2023

Codecov Report

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

Additional details and impacted files
@@            Coverage Diff             @@
##             develop      #34   +/-   ##
==========================================
  Coverage           ?   97.06%           
==========================================
  Files              ?        9           
  Lines              ?      307           
  Branches           ?        0           
==========================================
  Hits               ?      298           
  Misses             ?        9           
  Partials           ?        0           

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

Copy link
Collaborator

@rtimms rtimms left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks @ikorotkin looks great! can you also update the changelog

bpx/schema.py Outdated
if (parameter_class_name, model) in allowed_combinations:
return values
else:
raise ValueError(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we raise a warning instead of an error? think error might be aggressive and will end up being a barrier to #40

Copy link
Collaborator Author

@ikorotkin ikorotkin Sep 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any native way to raise a warning in pydantic?
It looks like it hasn't been implemented (see this issue).

EDIT: I'll do it using warnings.warn()

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed it to a warning. Updated tests and CHANGELOG.
Please feel free to suggest a better warning message if it sounds awkward :)

@ikorotkin ikorotkin requested a review from rtimms September 6, 2023 15:13
@rtimms rtimms merged commit 58930b3 into FaradayInstitution:develop Sep 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants