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.
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
SEI on cracks #2104
SEI on cracks #2104
Changes from 33 commits
b27ae28
23a0a6f
d17473a
e105d40
e4d93d8
e36602b
ecfcdc4
fea7d4c
5b321a7
6bfddd1
3fb424a
775626f
0ad8afe
a21484d
7ec5fc8
274439d
e106c1d
82d5800
17725e5
3dc6375
c78229a
6e7fbcf
0a93993
2ddb0c2
0080825
c628f95
f23b3ba
5f0258e
7ee18d8
1d6319e
ffdbb48
f6b979f
5ae0a6c
63a9c79
21f0374
4153103
9ad8b75
ebeffb1
51d1c4f
851b44c
2b77c81
5276a9b
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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.
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.
Fixed
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.
Please specify the activation energy and reference temperature here. The data is measured for a specific reference temperature and activation energy, so I don't think it makes sense to define them as separate parameters.
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.
Fixed
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.
Are all these parameters presented in O'Kane 2022? If so, should we rename this set to O'Kane 2022 rather than Chen 2020 plating? We can still keep the ref to Chen 2020 in
pybamm.citations()
but I think the new name would make it clearer to the reader which is the main reference.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.
I've gone further and deleted the entire Chen2020_plating chemistry. It was only there in the first place because none of the existing chemistries worked with the plating submodel. Now that OKane2022 exists, it's not needed anymore. OKane2020_Li_plating is still there as that is a published set of plating parameters, but none of the pre-defined chemistries use it.
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.
should be hard-coded
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.
Fixed (currently 0, sorry to disappoint!)
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.
as above
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.
Fixed