PyBaMM icon indicating copy to clipboard operation
PyBaMM copied to clipboard

Composite model for particle mechanics

Open mohammedasher opened this issue 1 year ago • 6 comments

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.

  • [x] 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)
  • [x] 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 avatar Oct 15 '24 11:10 mohammedasher

@brosaplanella Unable to request specific reviewer in draft mode.

mohammedasher avatar Oct 15 '24 11:10 mohammedasher

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).

brosaplanella avatar Oct 15 '24 12:10 brosaplanella

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!

mohammedasher avatar Oct 16 '24 09:10 mohammedasher

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.

brosaplanella avatar Oct 16 '24 14:10 brosaplanella

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.

DrSOKane avatar Oct 22 '24 15:10 DrSOKane

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'>

mohmdasher avatar Oct 23 '24 11:10 mohmdasher

@mohammedasher Are you still working on this?

kratman avatar Jan 07 '25 20:01 kratman

@mohammedasher Are you still working on this?

@kratman Yes, I'm working on this...

mohammedasher avatar Jan 08 '25 09:01 mohammedasher

@aabills does this overlap with what your did / are doing?

rtimms avatar Jan 08 '25 18:01 rtimms

I think this is mostly closed by #4698, though its hard to tell if I got everything.

aabills avatar Jan 28 '25 20:01 aabills

@mohammedasher Can you check and see if what you were intending to fix here works on pybamm 25.1.1?

aabills avatar Jan 28 '25 20:01 aabills

@mohammedasher Can you check and see if what you were intending to fix here works on pybamm 25.1.1?

Hi @aabills, I think your work on #4698 addresses this PR i.e. to make particle mechanics work for composite electrodes; as a next step I wanted to include thermal model, LLI and LAM for composite electrodes. I see that the changes you have made should address the thermal part as well, can you please confirm?

Thank you.

mohammedasher avatar Jan 29 '25 10:01 mohammedasher

Thermal models shouldn't be affected by the composite model because thermal model occurs at the electrode domain, while the composite model is at the particle domain, so temperature can just be broadcast to the composite parts of the models.

Thanks for your attempted contribution anyway! Sorry, we normally wouldn't fly through it when we have someone trying to contribute, but we had an urgent need to get this working.

aabills avatar Jan 29 '25 15:01 aabills