-
-
Notifications
You must be signed in to change notification settings - Fork 589
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 760 better processed variable #781
Conversation
Codecov Report
@@ Coverage Diff @@
## master #781 +/- ##
==========================================
- Coverage 98.44% 98.44% -0.01%
==========================================
Files 179 179
Lines 9951 10004 +53
==========================================
+ Hits 9796 9848 +52
- Misses 155 156 +1
Continue to review full report at Codecov.
|
Hey @tinosulzer looks good. Couple things might want to consider. A dedicated notebook explaining the different ways to access the solution data. At the moment all examples get you to use quickplot but it's useful to know you can use solution as a dictionary directly and behind the scenes processed variables are created. The problem with this approach is that you can't do solution.keys() to get the options and solution.data.keys() only contains ones already processed so it's not obvious to the user what to do necessarily hence why an example might be useful. Another wrinkle with the functionality is that when stepping the solution if you want to access the data after each step it doesn't get automatically updated when the solutions re appended together. This is a fairly subtle thing and a experienced user who is stepping can probably work out that they need to manually call solution.update on the variable after appending. But worth knowing about so again could go in the example. I'm happy to write it and you can review it tomorrow to see if I got it all |
@tinosulzer Feel free to change or move that new example. Also , do the notebooks get tested? If not I have a script that can do it on travis |
Thanks @TomTranter , that example notebook is very clear. Note in the example you chose the stepped solution stops at the voltage cut-off while the full solution goes right through it (this is a bug in scipy) so it might be better to use a shorter Some ways I can think of getting around those issues you picked up on
Examples do get tested on travis and you can run locally with
|
@tinosulzer I agree that solution.keys could get messy. It's not too much trouble to look in model.variables.keys. On the append solution that sounds good but would it add a lot of over head if the solution is appended to a lot. Might be better to leave that to. Up to you |
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.
Some issues identified and addressed
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.
This looks great! Thanks @tinosulzer and @TomTranter!
…ocessed-variable' into issue-760-better-processed-variable
Description
Refactored solution to make it a dictionary that contains all of the solution variables. This automatically creates
ProcessedVariable
objects when required, so that the solution can be obtained much more easily. The solution object can also be exported, either the full object (with.save
) or just an array with the data (with.save_data
)Fixes #760
Fixes #725
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: