-
-
Notifications
You must be signed in to change notification settings - Fork 553
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
Issue 3051 - bpx activation energies #3242
Issue 3051 - bpx activation energies #3242
Conversation
Added an additional commit to fix #3225 here. I was concerned separate PRs would cause a merge conflict. |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #3242 +/- ##
===========================================
+ Coverage 99.55% 99.60% +0.05%
===========================================
Files 253 253
Lines 19553 19541 -12
===========================================
- Hits 19466 19464 -2
+ Misses 87 77 -10
☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, can you add tests for coverage?
You could also reduce code duplication by defining the arrhenius function up front and just calling it everywhere e.g.
def arrhenius(Ea, T):
return exp(Ea / constants.R * (1 / T_ref - 1 / T))
and later
return arrhenius(Ea_D_e, T) * ...
I've not made a test before, so please double check it's ok! See separate discussion here #3225 about full testing requirements for BPX. |
The test looks good to me. The issues with the coverage seem to be that for various parameters it never reaches the |
I agree, I'm not sure why |
Can you run it in debug mode and check why the constant case does not reach the else? |
Sorry not sure what you mean by debug mode. However I tested it with print statements and the constant case does reach the else |
I think the issue is that in |
@brosaplanella I've added a test that I think addresses your previous comment, thanks! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me! I couldn't tell why the doc tests are failing though, @agriyakhetarpal do you have any insight?
CHANGELOG.md
Outdated
@@ -13,6 +13,7 @@ | |||
- Error generated when invalid parameter values are passed. ([#3132](https://github.com/pybamm-team/PyBaMM/pull/3132)) | |||
- Thevenin() model is now constructed with standard variables: `Time [s], Time [min], Time [h]` ([#3143](https://github.com/pybamm-team/PyBaMM/pull/3143)) | |||
- Fix SEI Example Notebook ([#3166](https://github.com/pybamm-team/PyBaMM/pull/3166)) | |||
- Fixed bug causing incorrect activation energies using `create_from_bpx()` ([#3242](https://github.com/pybamm-team/PyBaMM/pull/3242)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this up so they PRs are in decreasing order?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
There is a reST syntax warning/error in the installation instructions for WSL. I am adding a link to the erroneous lines because I cannot suggest changes directly here: Removing the extra colons in points 3 and 4 will fix it |
Thanks Agriya! @darryl-ad, can you resolve the conflict with CHANGELOG so tests run, and do the small fix Agriya suggested? |
@brosaplanella I've resolved the CHANGELOG conflict. All tests are passing on my end now. The erroneous lines in the docs that Agriya suggested aren't present in this branch - do I need to create an issue and branch for it? |
Looks good to me! We will need @tinosulzer to approve it as he requested changes. |
CHANGELOG.md
Outdated
@@ -8,8 +8,8 @@ | |||
- Implement the MSMR model ([#3116](https://github.com/pybamm-team/PyBaMM/pull/3116)) | |||
|
|||
## Bug fixes | |||
|
|||
- Fixed bug causing incorrect activation energies using `create_from_bpx()` ([#3242](https://github.com/pybamm-team/PyBaMM/pull/3242)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you move this line down so the PRs are sorted in decreasing order? Happy to merge after that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done!
Description
Fixes a bug where
E_a
was used as the alias for multiple different activation energies inbpx
, causing them to all take the final value. Also fixes bug wherereaction rate constant activation energy
was always set as 0.reaction rate activation energy
->reaction rate constant activation energy
inbpx
parameter search.Fixes # 3051 and # 3225
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.
Key checklist:
$ 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)$ python run-tests.py --all
(or$ nox -s tests
)$ 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: