-
-
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
Issue 688 add simulation class #693
Conversation
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 @Scottmar93 look good! just a couple of comments
Codecov Report
@@ Coverage Diff @@
## master #693 +/- ##
==========================================
- Coverage 98.52% 98.28% -0.24%
==========================================
Files 175 176 +1
Lines 8861 8982 +121
==========================================
+ Hits 8730 8828 +98
- Misses 131 154 +23
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.
Really good idea making this so that jupyter notebooks don't break. I would just say that it might be cleaner to keep a copy of the original model, and then add options to parameter values to return a new model (i.e. not inplace). Discretisation already does this. Then the sim class could have self.model, self.parametrized_model, self.discretised_model. I don't feel super strongly about this though if you want to keep as is :)
Also, we should make discretisation of an already discretised model just return the same model, not break (different issue)
Otherwise, agree with Rob's comments.
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.
Just the comment about parameters, the rest looks good to me.
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 but I am not able to pickle the simulation object which is unfortunate due to a local object:
'''import pybamm
import pickle
model = pybamm.lithium_ion.SPM()
sim = pybamm.Simulation(model)
sim.solve()
sim.plot()
with open('test_save_sim', 'wb') as f:
pickle.dump(sim, f)
'''
I think it is to do with the solver object and possibly some recursive object referencing. We will have to discuss how to save and load simulations in future but as it stands this looks good and should be merged
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 @Scottmar93 !
Description
Convenience class for interacting with the front-end of PyBaMM. I have set it up to try to be Jupyter notebook friendly in that things won't break if you try to build twice or solve without first building. It'll also automatically reset everything if you decide to change a spatial_method / parameter etc. Would be good to add some functionality on saving an loading later as well as allowing for experiment specific things to be set such as the current.
Fixes #688
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.
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: