-
-
Notifications
You must be signed in to change notification settings - Fork 546
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
Experiment refactor #1459
Experiment refactor #1459
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #1459 +/- ##
===========================================
+ Coverage 98.32% 98.34% +0.02%
===========================================
Files 294 295 +1
Lines 16854 17093 +239
===========================================
+ Hits 16571 16810 +239
Misses 283 283
Continue to review full report at Codecov.
|
…nto experiment-refactor
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 great, thanks @tinosulzer ! just a couple of minor comments
"id": "heavy-crisis", | ||
"metadata": {}, | ||
"source": [ | ||
"Alternatively, we can simulate many CCCV cycles. Here we simulate either 100 cycles or until the capacity is 80% of the initial capacity, whichever is first. The capacity is calculated by the eSOH model" |
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.
think the next (commented out) cell can be deleted and the "References" cell after it
"n = int(l//np.sqrt(l))\n", | ||
"m = int(np.ceil(l/n))\n", | ||
"\n", | ||
"fig, axes = plt.subplots(n,m,figsize=(10,10))\n", |
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.
very nice notebook! I think this will be popular. It doesn't need to be for this issue, but might be good to automate this e.g. sim.plot_summary_variables()
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, though it's not too hard to plot the variables (they're just numpy arrays, unlike the outputs of a regular solution) so I like leaving customizability to the user
@@ -38,19 +38,18 @@ def test_read_strings(self): | |||
"Run US06 (V) for 5 minutes", | |||
"Run US06 (W) for 0.5 hours", | |||
], | |||
{"test": "test"}, drive_cycles={"US06": drive_cycle}, | |||
{"test": "test"}, | |||
drive_cycles={"US06": drive_cycle}, |
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.
do we have an example anywhere of using a drive cycle in an experiment? or doesn't it actually work in a simulation yet? I'm not up to date with the progress there
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 it works but need to double check
Description
A loose collection of refactorings for simulating several cycles with degradation
Fixes #1018
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: