-
-
Notifications
You must be signed in to change notification settings - Fork 574
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
Timestamped experiments #2616
Timestamped experiments #2616
Conversation
…-team/PyBaMM into issue-1839-time-based-experiments
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #2616 +/- ##
========================================
Coverage 99.71% 99.71%
========================================
Files 248 248
Lines 18683 18736 +53
========================================
+ Hits 18629 18682 +53
Misses 54 54
☔ View full report in Codecov by Sentry. |
…-team/PyBaMM into issue-1839-time-based-experiments
@tinosulzer, following #2960, do we want the tags to be in the string or just as an option in |
I think just an option. I made it so you can pass a single tag or a list of tags |
I think this is ready to review |
pybamm/simulation.py
Outdated
@@ -256,6 +257,23 @@ def set_up_and_parameterise_model_for_experiment(self): | |||
) | |||
self.experiment_unique_steps_to_model[repr(op)] = parameterised_model | |||
|
|||
# Set up rest model if experiment has timestamps |
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.
How come we need a separate model for this? Can't we use the current model and set the current to be zero like we do for a rest?
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 thought it would be more robust to have a separate model for rest, in case the user only has, for example, power driven steps (unlikely, I know).
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.
ah ok, yeah, makes sense
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 @brosaplanella !
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.
Functionality looks good. In terms of API I am wondering whether timestamp
is the best name (what about start_time
?). Also wondering whether we should just always require a datetime object, instead of providing support for some (but not all) datetime formats. It's not too much work for someone to write their own function to create a datetime object from a string.
We could call it
Currently what we do is process the string by using Will make these changes and then I will merge. |
Description
Enable functionality to "timestamp" experiment steps so they only get triggered at that date & time. At the moment the timestamps can be defined as:
"Day D HH:MM:SS"
"YYYY-MM-DD HH:MM:SS"
I am using the
datetime
package so the flexibility with the format (e.g. padding zeros) is that ofdatetime
.Fixes #1839
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: