Issue 2418 dimensional - #2419
Merged
Merged
Conversation
valentinsulzer
marked this pull request as draft
November 2, 2022 00:50
…M into issue-2418-dimensional
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
…M into issue-2418-dimensional
…M into issue-2418-dimensional
valentinsulzer
marked this pull request as ready for review
February 20, 2023 04:26
rtimms
requested changes
Feb 20, 2023
rtimms
left a comment
rtimms
left a comment
Contributor
There was a problem hiding this comment.
Thanks, @tinosulzer, looks great! A few minor changes and a couple of things to check. This is going to make contributing models much simpler.
…am/PyBaMM into issue-2418-dimensional
Divided by 3600 to replace t0_cr
brosaplanella
approved these changes
Feb 23, 2023
brosaplanella
left a comment
brosaplanella
left a comment
Member
There was a problem hiding this comment.
Looks great, amazing job @tinosulzer! Just a few very minor comments.
| ] | ||
|
|
||
| self.algebraic[delta_phi] = sum_a_j_av - sum_a_j | ||
| self.algebraic[delta_phi] = (sum_a_j_av - sum_a_j) / self.param.a_j_scale |
Member
There was a problem hiding this comment.
Is dividing by self.param.a_j_scale just to have a better conditioned problem?
| phase_name = self.phase_name | ||
|
|
||
| if isinstance(self, pybamm.kinetics.NoReaction): | ||
| a = 1 |
Member
There was a problem hiding this comment.
Was this a bug or was it changed later one?(Asking because we have an issue with calendar ageing that could be related).
Member
Author
There was a problem hiding this comment.
for NoReaction it should have no effect on results since j=0 and a only ever appears as a * j
Co-authored-by: Ferran Brosa Planella <Ferran.Brosa-Planella@warwick.ac.uk>
rtimms
approved these changes
Feb 28, 2023
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Makes all models dimensional. Major changes:
scaleandreferenceattributes for variables, which can be set to make sure that variables are order 1. Algebraic equations also have to be multiplied by an appropriate number (e.g.L_x**2) to make them order 1 and hence pass the tolerance check.Most models are now a lot faster but some of the degradation models might be a bit slower, possibly less robust.
Fixes #2418
Fixes #2281
Fixes #2690
Fixes #1260
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:
$ flake8$ python run-tests.py --unit$ cd docsand then$ make clean; make htmlYou can run all three at once, using
$ python run-tests.py --quick.Further checks: