[Bug]: [24.5rc0] Simulations with `pybamm.Experiment` terminate after 24 h

[ejfdickinson]

PyBaMM Version

24.5rc0

Python Version

3.9.13

Describe the bug

Simulations using pybamm.Experiment with a voltage cut-off only appear to terminate with a time cut-off after 24 h.

I'm afraid I haven't had time to get into logs to figure out why this is happening!

Steps to Reproduce

The following simulation should proceed for approximately 100 h = 360000 s, until the specified voltage cut-off. However, it terminates after 24 h.

import pybamm

parameter_values = pybamm.ParameterValues("Chen2020")
model = pybamm.lithium_ion.DFN()
experiment = pybamm.Experiment(
    [
        "Discharge at 0.01C until 2.5 V",
    ]
)

sim = pybamm.Simulation(
    model,
    parameter_values=parameter_values,
    experiment=experiment,
)

sol = sim.solve()
sim.plot()

print(f'Last time step at {sim.solution["Time [s]"].entries[-1] / 3600} h')

Relevant log output

No response

[agriyakhetarpal]

Hi, @ejfdickinson – looks like a duplicate of https://github.com/pybamm-team/PyBaMM/issues/4155?

[ejfdickinson]

@agriyakhetarpal Yes, it is.

v24.1 does not do this, so it should be in your release notes for v24.5rc0.

Any recommendations on how to work around?

[MarcBerliner]

Any recommendations on how to work around?

@ejfdickinson you can include a duration in the experiment step, e.g.,

experiment = pybamm.Experiment(
    [
        "Discharge at 0.01C for 100 hours or until 2.5 V",
    ]
)

and it won't cut off at 24 hours.

[ejfdickinson]

Thanks @MarcBerliner !

[agriyakhetarpal]

v24.1 does not do this, so it should be in your release notes for v24.5rc0.

I think we should mark this as a breaking change, @valentinsulzer?

[rtimms]

IMO we should fix this before 24.5 proper. I don't know what logic changed or if there was a longer default before.

Update: the logic used to be

                if op_conds.type == "current":
                    # Current control: max simulation time: 3h / C-rate
                    Crate = op_conds.value / capacity
                    dt = 3 / abs(Crate) * 3600  # seconds
                else:
                    # max simulation time: 1 day
                    dt = 24 * 3600  # seconds

[rtimms]

Alternatively, should we require a duration to avoid unexpected behaviour?

[brosaplanella]

Why did we move away from that logic? As in, if we bring it back will it break anything?

[rtimms]

It got moved to the Step class instead of the Simulation class. It's easy to put the same logic back in for C-rate, but not current, as the default duration needs to return a float, not a symbol (see the commit above). It's fixable but needs a bit of work, and I'm leaning towards requiring a duration as it's less ambiguous. It would be a breaking change but easy for users to fix.

[ejfdickinson]

@rtimms For our use cases, a maximum duration would be fine, so long as it's declared on simulation exit like a "Time" stop rather than occurring silently. As noted in #4155 , 24 h is well within typical simulation times for normal experiments, so if the purpose is just to avoid hanging the solver due to an absurd duration, adding a few orders of magnitude to the default might be more sensible!

[valentinsulzer]

Should be fixed by #4239 . @ejfdickinson and @aabills can you make sure it works for your use case? Then can @kratman or @agriyakhetarpal cherry pick this into 24.5? Thanks