-
-
Notifications
You must be signed in to change notification settings - Fork 589
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
Add new electrolyte functions from Landesfeind 2019 #860
Add new electrolyte functions from Landesfeind 2019 #860
Conversation
import numpy as np | ||
|
||
|
||
def electrolyte_conductivity_Landesfeind2019_EC_DMC_1_1(c_e, T, T_inf, E_k_e, R_g): |
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 we get rid of T_inf, E_k_E and R_g as inputs to these functions in general? When they are needed, they can easily be called from within the function as pybamm.standard_parameters_lithium_ion.T_inf
, etc
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.
For R_g I would say yes. For T_inf could there be a situation where the reference temperature for different functions is different? We could also use **kwargs potentially although I don't know if that would work with the expression tree. But yeah if you see fewer problems I'm happy to make the changes
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 think we should definitely be open to the idea of different functional dependencies of these things, so being less prescriptive in terms of inputs is probably good
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.
if it's specific to a function then maybe it should be hard-coded into the function itself (same with E_k_e)
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.
Yeah maybe you are right, If there were a specific reference temperature it would probably be hardcoded into the function if there were other parameters dependent on 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.
Looks good, but can you double check the units in the diffusion coefficient are right?
...thium-ion/electrolytes/lipf6_Landesfeind2019/electrolyte_diffusivity_Landesfeind2019_base.py
Outdated
Show resolved
Hide resolved
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.
thanks @TomTranter ! looks good, pending checking the units
Codecov Report
@@ Coverage Diff @@
## develop #860 +/- ##
===========================================
+ Coverage 97.68% 97.74% +0.06%
===========================================
Files 209 217 +8
Lines 10587 10888 +301
===========================================
+ Hits 10342 10643 +301
Misses 245 245
Continue to review full report at Codecov.
|
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. Thanks Tom!
Description
Add new electrolyte functions for conductivity and diffusivity from Landesfeind 2019
Fixes # 859
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 docs
and then$ make clean; make html
You can run all three at once, using
$ python run-tests.py --quick
.Further checks: