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
#813 get t_eval from data #819
Conversation
Codecov Report
@@ Coverage Diff @@
## master #819 +/- ##
==========================================
+ Coverage 98.21% 98.22% +0.01%
==========================================
Files 181 180 -1
Lines 10241 10260 +19
==========================================
+ Hits 10058 10078 +20
+ Misses 183 182 -1
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 @rtimms , really glad this is fixed! I think this is the right approach too (automatic time setting in the simulation) - just see comment about when the warning is triggered
pybamm/simulation.py
Outdated
# of the data points. We only raise a warning here, as users | ||
# may genuinely only want the solution returned at some | ||
# specified points. | ||
if set(time_data).issubset(set(t_eval)) is False: |
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'm wondering if this will have problems with round-off errors? And should be also allow t_eval
with a sampling time smaller than the smallest gap in the data, but that doesn't hit exactly the time points?
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 good point re round-off errors. yeah that would probably be fine. not sure of the best way of testing this for all cases and give an informative message (e.g. if t_eval or the data non-uniform, would you then just check the biggest gap in t_eval and smallest in the data and just warn that the user should check to ensure they have enough resolution in t_eval?). perhaps doing that is best, as doing something naive like just checking the number of values might pass if e.g. t_eval had loads of points clustered near the start and not many later
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 yeah I hadn't thought of the non-uniform t_eval case. I think we can allow both options: checking that either the biggest gap in t_eval is smaller than the smallest in the data, or that each point in the data has a corresponding point in t_eval within some small error tolerance, should cover most cases
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've updated to just check max dt_eval vs min dt_data as this also catches the case where t_eval = t_data (since dt is equal here). let me know your thoughts and i'll change or merge as is
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 don't think this would always catch the case t_eval = t_data, for example if t_eval=t_data = [0,1,5,6]
then max(t_eval) = 4 > 1 = min(t_data)
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.
oh yeah good point haha
Description
When using the
Simulation
class with the current provided as data (i..e drive cycles)solve
now setst_eval
(if it isn't provided) to be the times in the data (non-dimensionlised). Ift_eval
is provided,solve
checks that it contains all of the points in the data, and raises a warning if it doesn't. Maybe this should raise an actual exception to be totally fool proof, but there may be cases where people only want the solution returned at a subset of points. Happy for input on this. I think if people are solving in the long-winded way (i.e. not usingSimulation
) then we should just trust them to pickt_eval
appropriately.Fixes #813
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: