-
-
Notifications
You must be signed in to change notification settings - Fork 563
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
Issue 2358 add temperature to experiment #2518
Issue 2358 add temperature to experiment #2518
Conversation
Codecov ReportBase: 99.69% // Head: 99.69% // Increases project coverage by
Additional details and impacted files@@ Coverage Diff @@
## develop #2518 +/- ##
========================================
Coverage 99.69% 99.69%
========================================
Files 271 271
Lines 19586 19594 +8
========================================
+ Hits 19526 19534 +8
Misses 60 60
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View 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, thanks! Just some implementation comments
if op_number == 0: | ||
experiment_parameter_values.update( | ||
{ | ||
"Initial temperature [K]": ambient_temperature, | ||
} | ||
) | ||
else: | ||
experiment_parameter_values.update( | ||
{"Ambient temperature [K]": self._original_temperature} | ||
) |
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.
add comment explaining why this needs to be done like this
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 added:
# If at the first operation, then the initial temperature
# should be the ambient temperature.
I thought this was the most intuitive behavior but I guess we don't absolutely have to have the cell start at the ambient temperature. Can either always start at 25oC or take whatever is in the parameter_values? What's your thoughts?
Making the changes for this today. Apologies for the delays |
…thub.com/pybamm-team/PyBaMM into issue-2358-add-temperature-to-experiment
I reformatted the checking for the temperature condition a bit so that we just match a regex. If we don't find the regex for temperature then we raise a Unfortunately, this does mean that cases typos in the temperature part of the string will only be raised as a warning but still pass through. I think this is the awkward side effect of not requiring the temperature to be in the string (for backwards compatibility). |
…thub.com/pybamm-team/PyBaMM into issue-2358-add-temperature-to-experiment
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, I think this is the correct level of error-checking while still allowing back-compatibility. Can you update changelog?
Nice thanks, have updated |
Description
Extension of the experiment class to allow control of the temperature.
Input form is now:
"Charge at C/3 for 1 hour at 25oC (1 second period)"
Open to suggestions on where best to have the temperature.
Also added a global temperature for the experiment and made some minor modifications to the ECM to ensure it is compatible with the temperature control.
Getting a minor error in the docs when building which appears to also be in develop:
Fixes #2358
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: