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

Composite model for particle mechanics #4516

Draft
wants to merge 39 commits into
base: develop
Choose a base branch
from

Conversation

mohammedasher
Copy link

Description

Implementing the composite model for particle mechanics: The phase in the current particle mechanics implementation is set to 'primary' by default. This is being updated to support independent phases (e.g., primary/secondary such as Gr/Si). To accommodate these changes, the associated files in full_battery_models, interface, SEI, and particle modules have been updated, along with modifications to the Chen2020 parameter set.

This is part of the work by Bonkile et al.

Fixes # (issue)

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

@mohammedasher
Copy link
Author

@brosaplanella Unable to request specific reviewer in draft mode.

@brosaplanella
Copy link
Member

Hi Asher! The style tests are failing, this is because you have some defined variables/function that are not used. This is an issue it can't be automatically fixed by the pre-commit bot, so you will need to do it manually. You can find what's causing the issue if you check the failing test (https://github.com/pybamm-team/PyBaMM/actions/runs/11345431722/job/31553629641?pr=4516) and scroll down (you need to start reading at line 46).

@mohammedasher mohammedasher force-pushed the composite_mechanics branch 2 times, most recently from 344069e to 029cdee Compare October 15, 2024 13:16
@mohammedasher
Copy link
Author

Hi @brosaplanella,
I've addressed the style issues, and now the remaining problem is with the unit tests. Although multiple tests are failing, they all stem from two types of errors:

  1. AttributeError: 'set' object has no attribute 'values'
  2. AttributeError: 'ReactionDriven' object has no attribute 'phase_name'
    Could you advise on how I should proceed with resolving these? Thank you!

@brosaplanella
Copy link
Member

Hi! The latter is because the ReactionDriven porosity model (reaction_driven_porosity.py) is not available for composite materials, that's why it breaks. I need to dig a bit more regarding the first issue.

BTW, it's better to not force push. I believe the issue you encountered is that the pre-commit bot made some style changes. In those cases you have to pull first and then push.

@DrSOKane
Copy link
Contributor

DrSOKane commented Oct 22, 2024

Hi! The latter is because the ReactionDriven porosity model (reaction_driven_porosity.py) is not available for composite materials, that's why it breaks.

I made a PR that fixes this, which Sunil is already using (he requested it). @mohammedasher you can go to this branch of my fork of PyBaMM and merge the changes into your branch.

@mohmdasher
Copy link

mohmdasher commented Oct 23, 2024

Thank you @DrSOKane; merging reaction_driven_porosity from your branch significantly reduced the number of failed tests.
Now, the errors are:

  • AttributeError: 'set' object has no attribute 'values'
  • AttributeError: 'ReactionDriven' object has no attribute 'phase_name'
    Can this be re-scoped to self.phase_name??
  • pybamm.expression_tree.exceptions.ModelError: Missing variable for submodel 'porosity': 'Negative total SEI thick...'
  • Failed: DID NOT RAISE <class 'pybamm.expression_tree.exceptions.OptionError'>

Copy link
Member

@brosaplanella brosaplanella left a comment

Choose a reason for hiding this comment

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

Some comments that should help fix the bugs. Regarding the one about the "Negative total SEI thickness [m]" I have not been able to find where the issue is. Maybe @DrSOKane knows, as he has been working with the model recently.

return pybamm.FunctionParameter(
f"{self.phase_prefactor}{Domain} electrode volume change",
{
f"{self.phase_prefactor}{Domain} electrode volume change",
Copy link
Member

Choose a reason for hiding this comment

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

I think this one is causing the set has no values attribute, as you forgot to pass the variable in the inputs.

Comment on lines +141 to +142
PublicPyBaMM_Composite/
PyBaMM/
Copy link
Member

Choose a reason for hiding this comment

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

These two are repository specific, I would suggest to remove them

@@ -37,7 +38,9 @@ def get_coupled_variables(self, variables):
L_pl_k = variables[f"{Domain} lithium plating thickness [m]"]
L_dead_k = variables[f"{Domain} dead lithium thickness [m]"]
L_sei_cr_k = variables[f"{Domain} total SEI on cracks thickness [m]"]
roughness_k = variables[f"{Domain} electrode roughness ratio"]
roughness_k = variables[
f"{Domain} electrode {self.phase_name}roughness ratio"
Copy link
Member

Choose a reason for hiding this comment

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

The issue with the phase_name is that this class does not take a phase as argument (see for example how loss_active_material.py does it. However, I don't understand how this model works: you have different values for different phases for the roughness, but the SEI on cracks thickness is only one. Shouldn't you just have one roughness ratio overall?

Copy link
Author

Choose a reason for hiding this comment

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

@DrSOKane could you please advise on how I can go about with introducing phases in the reaction-driven-porosity model? Most of the remaining errors are stemming from this file. Thank you.

Copy link
Contributor

@DrSOKane DrSOKane Dec 19, 2024

Choose a reason for hiding this comment

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

Hi @mohammedasher, you need to move the roughness ratio inside the loop for phase in phases:. Something like this should work:

roughness_k = variables[f"{Domain} total {phase_name}SEI on cracks thickness [m]"]

Then, delete the reference to roughness_k outside the loop (line 36 in my version). phase_name is defined at the start of the loop for phase in phases: so that gets around the problems Ferran rightly pointed out.

Copy link
Author

@mohammedasher mohammedasher Dec 20, 2024

Choose a reason for hiding this comment

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

Hi @DrSOKane....I will define a new for phase in phases: loop; apart from the roughness_k variable, L_sei_k = variables[f"{Domain} total SEI thickness [m]"] also runs into error when the domain is negative (negative electrode). I tried L_sei_k = variables[f"{Phase}: {Domain} total SEI thickness [m]"]', but it did not help.

Copy link
Contributor

Choose a reason for hiding this comment

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

@mohammedasher the code you need is here. Uncomment lines 63-65 and deleted line 36, and it should work.

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