Fixing an error occurring when Heavisides are used and a model is initialised from a previous state #5075
Conversation
…at it works when a model is initialised from a previous state
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #5075 +/- ##
========================================
Coverage 99.12% 99.12%
========================================
Files 304 305 +1
Lines 23576 23608 +32
========================================
+ Hits 23369 23401 +32
Misses 207 207 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
@isaacbasil Can you also add a test to catch this issue and then add an entry in the bugs section of the changelog?
…t) when the solver is called from non-zero time
|
@kratman just added this, does it look ok to you? |
| def test_discontinuity_removed_at_nonzero_initial_time(self): | ||
| # Test that discontinuity caused by Heaviside(t) is removed when solver called with non-zero initial time | ||
| model = pybamm.BaseModel() | ||
| u = pybamm.Variable("u") | ||
| v = pybamm.Variable("v") | ||
| model.rhs = {v: -1 * (pybamm.t < 1)} | ||
| model.algebraic = {u: v - 1 - u} | ||
| model.initial_conditions = {v: 1, u: 0} | ||
| disc = pybamm.Discretisation() | ||
| disc.process_model(model) | ||
| solver = pybamm.IDAKLUSolver() | ||
| sol1 = solver.solve(model, t_eval=[0, 1]) | ||
|
|
||
| model.set_initial_conditions_from(sol1) | ||
| sol2 = solver.solve(model, t_eval=[1, 2]) |
There was a problem hiding this comment.
Unit tests should assert something at the end.
Options:
- Assert that there are no errors or warnings raised by the code (if the tested code is just catching an issue)
- Check that a value matches what you would expect
There was a problem hiding this comment.
Ah sorry it's the first time I've made one. I just added an assert statement to assert that there are no errors.
There was a problem hiding this comment.
I don't think this test is quite what we want to cover this issue. The problem is that it just runs a simulation and confirms nothing crashes, but it does not actually check for the error you are fixing. For this to even test anything, incorrect discontinuities would have to force the solver to return None.
A better approach is to probably extract the following code to a function:
# make sure they are increasing in time
discontinuities = sorted(discontinuities)
# remove any identical discontinuities
discontinuities = [
v
for i, v in enumerate(discontinuities)
if (
i == len(discontinuities) - 1
or discontinuities[i] < discontinuities[i + 1]
)
and v > t_eval[0]
]
# remove any discontinuities after end of t_eval
discontinuities = [v for v in discontinuities if v < t_eval[-1]]This does not really depend on the model or inputs, so you can pass in an array of discontinuities and confirm the case for t_eval[0] works as expected. What you want to do is write the test, get it working, then temporarily undo your change and confirm that the test starts to fail.
A good rule of thumb for testing is that if the section of the code you are testing is difficult to test, then you probably need to make a function that is easy to test.
There was a problem hiding this comment.
I don't think I understand. I'm not sure how I would test something meaningful with the function extracted like that? In other words, I don't know how I would make the test interact with the base_solver.py file.
Another idea I had was to do something like this, which tests that the _get_discontinuity_start_end_indices function correctly removes the discontinuity:
model = pybamm.BaseModel()
u = pybamm.Variable("u")
v = pybamm.Variable("v")
model.rhs = {v: -1 * (pybamm.t < 1)}
model.algebraic = {u: v - 1 - u}
model.initial_conditions = {v: 1, u: 0}
disc = pybamm.Discretisation()
disc.process_model(model)
solver = pybamm.BaseSolver()
t_eval = [1, 2]
solver.set_up(model, inputs=None, t_eval=t_eval, ics_only=False)
start_indices, end_indices, t_eval = solver._get_discontinuity_start_end_indices(model, inputs=None, t_eval=t_eval)
assert len(t_eval) == 2
There was a problem hiding this comment.
Hi @kratman would you be able to give me another pointer so that I can finish this off?
| sol1 = solver.solve(model, t_eval=[0, 1]) | ||
|
|
||
| model.set_initial_conditions_from(sol1) | ||
| sol2 = solver.solve(model, t_eval=[1, 2]) |
There was a problem hiding this comment.
Note: The pre-commit failures are from here, since you don't use sol2 for anything, it is a violation of the style/formatting guidelines. Fixing the test will also fix the pre-commit failure
Description
Notes
Fixes # (issue)
Type of change
Please add a line in the relevant section of CHANGELOG.md to document the change (include PR #)
Important checks:
Please confirm the following before marking the PR as ready for review:
nox -s pre-commitnox -s testsnox -s doctestsMWE