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

Add support for well-mixed, blended electrodes that contain more than one active material #33

Merged

Conversation

ikorotkin
Copy link
Collaborator

Following the implementation idea from the discussions, I basically replaced negative_electrode: Electrode with a Union:
negative_electrode: Union[Electrode, Dict[str, Electrode]], so that both single and blended electrodes will be supported.

I.e., for non-blended electrodes, nothing changes in the standard:

"Negative electrode": {
    "Particle radius [m]": 4.12e-06,
    "Thickness [m]": 5.62e-05,
    ...
}

And blended electrodes can be defined as

"Negative electrode": {
    "Primary": {
        "Particle radius [m]": 4.12e-06,
        "Thickness [m]": 5.62e-05,
        ...
    },
    "Secondary": {
        "Particle radius [m]": 4.12e-06,
        "Thickness [m]": 5.62e-05,
        ...
    },
    ....
}

Related to issue #23.

@codecov
Copy link

codecov bot commented Jun 20, 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      #33   +/-   ##
==========================================
  Coverage           ?   96.56%           
==========================================
  Files              ?        9           
  Lines              ?      262           
  Branches           ?        0           
==========================================
  Hits               ?      253           
  Misses             ?        9           
  Partials           ?        0           

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

@ejfdickinson
Copy link
Collaborator

I don't get how this is supposed to work for those properties that describe the electrode composite, rather than the materials themselves. What happens if you set two different values for "Thickness [m]" between two blend components, for instance?

I would see the following as being strictly properties of the electrode that should not be stated more than once:

  • "Thickness [m]"
  • "Conductivity [S.m-1]"
  • "Porosity"
  • "Transport efficiency"

@ejfdickinson
Copy link
Collaborator

As a second remark, I think it's an irritation of the present BPX that it's necessary to define the mass loading of active components indirectly, via the specification of a surface area and a particle radius combined with the assumption of spherical particles.

I think it would be more comprehensible to be able (optionally) to introduce some ability to specify e.g. a mass fraction and corresponding specific capacity of each blend component, and make the active volumetric surface area a dependent quantity under the equivalent spherical particle assumption.

@rtimms
Copy link
Collaborator

rtimms commented Jun 20, 2023

@ejfdickinson totally agree with your comment about mass loading. It feels backwards at the moment. I think we should specify the active material volume fraction directly. In your experience, what do you think is actually the most sensible set of parameters to specify from an experimental point of view? The current standard is very much from the model POV and I think it might be more useful to approach it from the other direction.

Splitting electrode properties into "chemistry" and "not" seems sensible too, so we can do something like

"Negative electrode": {
    "Thickness [m]": 5.62e-05,
    "Primary": {
        "Particle radius [m]": 4.12e-06,
        ...
    },
    "Secondary": {
        "Particle radius [m]": 4.12e-06,
        ...
    },
    ....
}

I think we can make separate classes for ElectrodeChemsitry and ElectrodeOther (not sure what the best name is) and then allow the appropriate unions. Validation will be a bit more complex.

@ikorotkin
Copy link
Collaborator Author

ikorotkin commented Jun 20, 2023

Thank you for your comments, this totally makes sense!

As @rtimms suggested, I split the Electrode class into two: ElectrodeChemistry and ElectrodeOther, and derived two new classes from them: Electrode (equivalent to the old Electrode class) and ElectrodeBlended.

As @ejfdickinson suggested, I moved the four properties out of ElectrodeChemistry class.

The JSON now may look like this:

"Positive electrode": {
    "Thickness [m]": 75.6e-6,
    "Conductivity [S.m-1]": 0.18,
    "Porosity": 0.335,
    "Transport efficiency": 0.1939,
    "Chemistry": {
        "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 the new field Chemistry for blended electrodes. Without it, the validator will complain about the names Primary and Secondary, that extra field names are not permitted.

Let me know if there is a better way of implementing this, and if I need to move more fields out of Chemistry.

@valentinsulzer
Copy link

This seems like a good solution. For naming, instead of ElectrodeChemistry and ElectrodeOther, can I suggest Electrode and Particle.

@ikorotkin
Copy link
Collaborator Author

That sounds like a good idea to me, thanks @tinosulzer.
Renamed the classes. Also, renamed the Chemistry field to Particle. Now JSON file can look like this:

"Positive electrode": {
    "Thickness [m]": 75.6e-6,
    "Conductivity [S.m-1]": 0.18,
    "Porosity": 0.335,
    "Transport efficiency": 0.1939,
    "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 that the old structure (for single electrode chemistry) is still valid:

"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]},
    "Conductivity [S.m-1]": 215.0,
    "Surface area per unit volume [m-1]": 383959,
    "Porosity": 0.25,
    "Transport efficiency": 0.125,
    "Reaction rate constant [mol.m-2.s-1]": 1e-10,
    "Maximum concentration [mol.m-3]": 33133,
    "Minimum stoichiometry": 0.01,
    "Maximum stoichiometry": 0.99,
},

This PR won't break old BPX files.

And I feel like the Particle class will be useful for SPM and SPMe models (issue #29).

These are just suggestions, feel free to correct me.

@ejfdickinson
Copy link
Collaborator

Nice work @ikorotkin !

I think that this solution looks good, and I'm happy with the revised nomenclature. Shall we think about widening the set of test cases so that pass/fail can be checked against correct or pathological JSON instances expressing blended electrodes?

I'd suggest that we consider merging this PR and then branching the discussion point raised by me and @rtimms (on replacing Surface area per unit volume [m-1] with a mass- or volume-fraction-based definition, #33 (comment)) into a new issue with a new PR.

@ejfdickinson
Copy link
Collaborator

Comments related to the replacement of Surface area per unit volume [m-1] are now replicated (branched) in #41 .

No further issues on the content of this PR, though the new functionality needs some suitable tests.

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 good! can you add an entry to the changelog.

@codecov-commenter
Copy link

Welcome to Codecov 🎉

Once merged to your default branch, Codecov will compare your coverage reports and display the results in this comment.

Thanks for integrating Codecov - We've got you covered ☂️

@ikorotkin ikorotkin merged commit 679e153 into FaradayInstitution:develop Sep 11, 2023
12 checks passed
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.

5 participants