-
-
Notifications
You must be signed in to change notification settings - Fork 491
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
Benchmarks for model X parameter set combinations #2086
Benchmarks for model X parameter set combinations #2086
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2086 +/- ##
========================================
Coverage 99.38% 99.38%
========================================
Files 348 348
Lines 19255 19255
========================================
Hits 19136 19136
Misses 119 119 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.
Hi Vaibhav! It looks pretty good, but there are some changes I suggested (mostly to make use of parameters in benchmarks so you can simplify your code a lot).
I couldn't attend last meeting, so if any of my comments contradicts anything you agreed previously do let me know (and apologies in advanced).
I agree with Ferran's comments about reducing code duplication. You could also use a subclass to do this, e.g. class BaseTiming:
def time_setup(self):
self.param.process_model(self.model)
compute_discretisation(self.model, self.param).process_model(self.model)
class TimeBuildSPMNCA_Kim2011(BaseTiming):
def __init__(self):
self.param = pybamm.ParameterValues("NCA_Kim2011")
self.model = pybamm.lithium_ion.SPM() For the Mohtat2020 model, it isn't working because the initial concentrations are for a completely discharged cell. Just skip that one for now and we can come back to it |
3d97aad
to
c115e02
Compare
Thank you for the review @tinosulzer, @brosaplanella! I have parameterized the benchmarks and I am still looking into the solvers. Could you please review it again and give your feedback. Also, keeping the voltage same for every parameter did not throw any error, should I keep it the same or change it to match every parameter's lower voltage. |
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.
Nice, it looks much cleaner now!
For the voltage, it would be better to test down to the right voltage specified in the parameters so we ensure we have a full discharge. You can just get the value for |
[ | ||
"Marquis2019", | ||
"ORegan2021", | ||
"NCA_Kim2011", | ||
"Prada2013", | ||
"Ai2020", | ||
"Ramadass2004", | ||
"Mohtat2020", | ||
"Chen2020", | ||
"Chen2020_plating", | ||
"Ecker2015", | ||
], |
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.
Could you make a global variable with a list of these parameter sets outside the classes and use that in all the functions to reduce code duplication
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, looks good!
@priyanshuone6 do you have any further comments on the PR or shall I merge it? |
Let’s merge this |
Description
Added benchmarks for
SPM
,SPMe
, andDFN
with all the parameter sets (except Sulzer2019 and Xu2019).Some concerns -
DFN
model withMohtat2020
parameter sets for the benchmarks (commented code). I tried varyingdt_max
and used different solvers but nothing worked. Here is a reproducible example for the same-The error-
ScikitsDaeSolver
for some benchmarks, will this create a problem or is it safe to use different solvers for similar benchmarks?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: