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

[WIP] updates for BPX v0.4.0 compatibility #3414

Merged
merged 7 commits into from
Jan 25, 2024
Merged

[WIP] updates for BPX v0.4.0 compatibility #3414

merged 7 commits into from
Jan 25, 2024

Conversation

rtimms
Copy link
Contributor

@rtimms rtimms commented Oct 5, 2023

Description

Add support for blended electrodes and user-defined parameters in BPX. Create parameters from BPX file in a loop using partial.

PyBaMM now requires bpx>=0.4.0

Note: BPX set initial concentrations based on SOC, which is not yet supported in pybamm for blended electrodes (see #2682). If a user supplies a "blended electrode" BPX file then they must manually provide initial concentrations after the ParameterValues class has been created (or supply them in the "user-defined" section).

Fixes #3679

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

@rtimms rtimms marked this pull request as draft October 5, 2023 16:06
@rtimms
Copy link
Contributor Author

rtimms commented Jan 10, 2024

@Saransh-cpp I'd also like to get this into 24.1 after review (so long as we can review and merge fairly quickly - I don't want to hold things up too much)

Copy link

codecov bot commented Jan 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (21c460f) 99.59% compared to head (c8264b8) 99.59%.
Report is 2 commits behind head on develop.

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #3414   +/-   ##
========================================
  Coverage    99.59%   99.59%           
========================================
  Files          258      258           
  Lines        20827    20820    -7     
========================================
- Hits         20742    20736    -6     
+ Misses          85       84    -1     

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

@rtimms rtimms marked this pull request as ready for review January 11, 2024 15:22
Copy link
Member

@Saransh-cpp Saransh-cpp left a comment

Choose a reason for hiding this comment

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

@rtimms ideally no new features (and especially no new breaking changes) should be added to a release after the release candidate is out. I think we should not add this to v24.1, given that this is not a bug fix.

@rtimms
Copy link
Contributor Author

rtimms commented Jan 12, 2024

@Saransh-cpp ok, thanks, let’s leave this out

@rtimms
Copy link
Contributor Author

rtimms commented Jan 13, 2024

there is new functionality in BPX v0.4 that won’t work with pybamm until this is merged, so we should either merge this or pin BPX < 0.4

this isn’t a huge breaking change in that is immediately fixed by updating the BPX version, but happy to wait to add it as a new feature in the next version

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.

Looks good, my main coverage is that coverage needs to be improved. Happy either for this to make it in the release or waiting until 24.5 (and pinning version for this release).

@rtimms rtimms merged commit 3253bd3 into develop Jan 25, 2024
38 of 41 checks passed
@rtimms rtimms deleted the bpx-v040 branch January 25, 2024 10:25
js1tr3 pushed a commit to js1tr3/PyBaMM that referenced this pull request Aug 12, 2024
* use partial to get parameters in loop, add user-defined

* add support for blended electrodes

* add tests for bpx-0-4-0 for blended electrodes and user-defined parameters

* update CHANGELOG

* coverage
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.

ParameterValues.create_from_bpx - support BPX v0.4
3 participants