replacing rounded faraday constant with exact value in bpx.py#4290
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #4290 +/- ##
========================================
Coverage 99.45% 99.46%
========================================
Files 288 288
Lines 22091 22079 -12
========================================
- Hits 21971 21960 -11
+ Misses 120 119 -1 ☔ View full report in Codecov by Sentry. |
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
agriyakhetarpal
left a comment
There was a problem hiding this comment.
Thanks for the PR, @dion-w! Just a small suggestion on the style for the CHANGELOG entry (maybe we should catch things like this with a Markdown linter as well).
|
FYI you can do |
I agree, using a constant from a library is better than updating this file. One other thing to check would be if the BPX standard specifies a value for Faraday constant. Sometimes standards specify the precision to ensure that things are constant between implementations |
Co-authored-by: Eric G. Kratz <kratman@users.noreply.github.com>
Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com>
|
Re-requested review/approval from @kratman since he previously requested changes |
…-team#4290) * updated faraday constant to exact value in bpx.py * Update CHANGELOG.md * Update CHANGELOG.md Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com> * Update CHANGELOG.md * Update pybamm/parameters/bpx.py Co-authored-by: Eric G. Kratz <kratman@users.noreply.github.com> * Update CHANGELOG.md Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com> --------- Co-authored-by: Agriya Khetarpal <74401230+agriyakhetarpal@users.noreply.github.com> Co-authored-by: Eric G. Kratz <kratman@users.noreply.github.com> Co-authored-by: Arjun Verma <arjunverma.oc@gmail.com>
Description
The faraday constant has an exact value, calculated by the exact values of the avogadro constant and the elementray charge [see here]. Using a rounded value may introduce systemic errors further down the line that can be easily avoided with this new value.
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.
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: