-
-
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
#1080 require t_eval #1086
#1080 require t_eval #1086
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1086 +/- ##
===========================================
- Coverage 97.78% 97.78% -0.01%
===========================================
Files 242 242
Lines 12662 12658 -4
===========================================
- Hits 12381 12377 -4
Misses 281 281
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.
Thanks Rob, looks great!
pybamm/simulation.py
Outdated
"list [t0, tf] where t0 is the initial time and tf is the " | ||
"final time. " | ||
"For a constant current (dis)charge the suggested 't_eval' " | ||
"is [0, 4000/C] where C is the C-rate." |
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.
Played around a little bit with this and I think we should suggest something closer to 3600 (e.g. 3700)
Description
Make
t_eval
are required argument insim.solve()
when not using an experiment or drive cycleFixes #1080
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: