Return event state

[rtimms]

Description

UPDATE (16/12/20): the event time and state are now always returned as part of solution.t and solution.y. The solver option for this has been removed.

Add a solver option to return the event time and state as part of solution.t and solution.y.

Also changes the way stepping works in experiments when an event is triggered so that the solution gets back on to the correct reporting period. e.g. if the period is 30s and an event is triggered at 32s we now return the times [0, 30, 32, 60] instead of [0, 30, 32, 62]

Open to suggestions on the option name and on how I've implemented this. Question: should the default be True? I left it as False as typically it seems that solvers don't return the event in this way. Maybe it should be true for experiments at least? I think it is important to get the event time and state back when looking at the solution, especially for doing calculations after the fact.

See here that you now hit the event spot on, regardless of the period

import pybamm

pybamm.set_logging_level("INFO")
experiment = pybamm.Experiment(["Discharge at 1C until 3.6V", "Rest for 1 hour"] * 2)
model = pybamm.lithium_ion.DFN()
sim = pybamm.Simulation(model, experiment=experiment)

solvers = [pybamm.CasadiSolver(), pybamm.CasadiSolver(return_event=True)]
sols = []
times = []
for solver in solvers:
    sol = sim.solve(solver=solver)
    sols.append(sol)
    times.append(sol["Time [s]"].entries)

pybamm.dynamic_plot(
    sols, ["Terminal voltage [V]"], labels=["return_event=False", "return_event=True"]
)

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.

• New feature (non-breaking change which adds functionality)
• Optimization (back-end change that speeds up the code)
• Bug fix (non-breaking change which fixes an issue)

Key checklist:

• No style issues: $ flake8
• All tests pass: $ python run-tests.py --unit
• The documentation builds: $ cd docs and then $ make clean; make html

You can run all three at once, using $ python run-tests.py --quick.

Further checks:

• Code is commented, particularly in hard-to-understand areas
• Tests added that prove fix is effective or that feature works

[codecov]

Codecov Report

Merging #1300 (956deb5) into develop (c573c78) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop    #1300   +/-   ##
========================================
  Coverage    98.10%   98.10%           
========================================
  Files          272      272           
  Lines        15229    15236    +7     
========================================
+ Hits         14940    14947    +7     
  Misses         289      289           

Impacted Files	Coverage Δ
pybamm/solvers/idaklu_solver.py	89.38% <ø> (ø)
pybamm/simulation.py	96.95% <100.00%> (ø)
pybamm/solvers/base_solver.py	99.03% <100.00%> (+0.01%)	⬆️
pybamm/solvers/casadi_solver.py	99.48% <100.00%> (ø)
pybamm/solvers/processed_variable.py	99.61% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c573c78...956deb5. Read the comment docs.

[rtimms]

one problem this has caused is that doing the extra single step to go from the event time to the next time based on the period adds extra sub solutions. this makes it so that there can be more sub solutions that instructions (e.g. the cccv example now has 19 sub solutions instead of 15, which breaks the custom plot of the discharge).

[valentinsulzer]

I think return_events should always be True. Actually I don't see the need for the option "False". If events happen, the solution.t you get out will be different from t_eval anyway.
Don't worry too much about sub-solutions, I want to reformat this with a concept of "cycles" anyway

[brosaplanella]

I agree with Tino. In addition, if you want the solution at the evenly spaced times you provided (which probably will be for storing or further processing), you can always evaluate the processed variables at the desired points.

[rtimms]

thanks, I've removed this as an option and the solvers now always return the event time and state. I've left the step in that gets you back to the times defined by the period based on the initial time. the reason I wanted to do this was incase people were doing e.g. lots of cycles and wanted to use a long period as they are only interested in the solution at the end of the cycle. if you have a long period then when an event is triggered you might get thrown a long way off. at least this way you should be able to trust the solution at the returned times, and aren't getting killed by interpolation error.

[brosaplanella]

Looks great, thanks Rob!

About the mac tests, I think that this is a GitHub issue (they are doing some maintenance this week which affects mac-os Actions).

[DrSOKane]

I tried merging this into my LithiumPlating branch and got the following error:

---

AttributeError Traceback (most recent call last)
in
46
47 sim = pybamm.Simulation(model=model, parameter_values=params, experiment=exp)
---> 48 sim.solve()
49 sim.solution.save("/mnt/e/pybamm_data/Plating_1E-8_2E-6_v3.pkl")
50
~/PyBaMM-GEM/pybamm/simulation.py in solve(self, t_eval, solver, check_model, **kwargs)
462 * self._solution.timescale_eval
463 )
--> 464 self.step(dt_event, solver=solver, **kwargs)
465 # Make sure we take at least 2 timesteps
466 npts = max(int(round(dt / exp_inputs["period"])) + 1, 2)
~/PyBaMM-GEM/pybamm/simulation.py in step(self, dt, solver, npts, save, **kwargs)
514
515 self._solution = solver.step(
--> 516 self._solution, self.built_model, dt, npts=npts, save=save, **kwargs
517 )
518
~/PyBaMM-GEM/pybamm/solvers/base_solver.py in step(self, old_solution, model, dt, npts, external_variables, inputs, save)
838
839 # Assign times
--> 840 solution.set_up_time = set_up_time
841 solution.solve_time = timer.time()
842
AttributeError: 'NoneType' object has no attribute 'set_up_time'

[brosaplanella]

@DrSOKane not sure if this is the cause of the problem you are having, but you would get a similar error if you haven't solved your model yet and ask for the set_up_time. Could you check if your model has been solved?

[DrSOKane]

@brosaplanella I haven't asked for the set up time.

The line raising the error is 840 of base_simulation.py. solution, the object Python is saying is NoneType, is defined on line 837. If solution is NoneType, that implies the integration on line 837 didn't work.

[valentinsulzer]

dt_event can be negative here if switching from a longer period (e.g. 180s) to a shorter period (e.g. w60s), since in that case (self._solution.t[-1] - self._solution.t[-2]) * self._solution.timescale_eval (e.g. 150s) can be longer than the new period even though it is shorter than the old period

[valentinsulzer]

This error arises when the casadi solver receives a time [t, t+dt] where dt is negative.
There is no check for this currently, so the first thing to add is an error message if dt < 0 in BaseSolver.step.
The place a negative dt is being passed is when correcting to get back to the correct time period here.

To be honest, I'm not sure we should be doing this to get back on the correct time period? e.g. in your [0,30,32,60,90] example, from the point of view of the phase after the event, the times will be [28,30,30,...] which seems a bit weird

[rtimms]

ah good catch, thanks. i’m happy for that step to get to the original period to be removed - it messes with the sub solutions anyway. my thinking was that it would be nice to get the solution at the times you would expect based on starting at t=0 with a given period, and that the event times would just be appended in.

i probably won’t get round to this until the new year, so if someone else wants this to be merged into develop sooner then please go ahead and make the changes

[rtimms]

thanks @tinosulzer ! I’ll revert the cccv script and update the changelog etc. after the holidays