Fix bug with identical steps with different end times (pybamm-team#3516)

* fix bug with identical steps with different end times

* add copy method for steps

* undo testing changes

* fix failing tests

* update CHANGELOG

* remove copy method as it is unused

* remove raw termination as unused

pybamm/experiment/experiment.py

@@ -78,7 +78,7 @@ def __init__(
         self.operating_conditions_cycles = operating_conditions_cycles
         self.cycle_lengths = [len(cycle) for cycle in operating_conditions_cycles]
 
-        operating_conditions_steps_unprocessed = self._set_next_start_time(
+        self.operating_conditions_steps_unprocessed = self._set_next_start_time(
             [cond for cycle in operating_conditions_cycles for cond in cycle]
         )
 
@@ -89,7 +89,7 @@ def __init__(
         self.temperature = _convert_temperature_to_kelvin(temperature)
 
         processed_steps = {}
-        for step in operating_conditions_steps_unprocessed:
+        for step in self.operating_conditions_steps_unprocessed:
             if repr(step) in processed_steps:
                 continue
             elif isinstance(step, str):
@@ -106,7 +106,7 @@ def __init__(
 
         self.operating_conditions_steps = [
             processed_steps[repr(step)]
-            for step in operating_conditions_steps_unprocessed
+            for step in self.operating_conditions_steps_unprocessed
         ]
 
         # Save the processed unique steps and the processed operating conditions

pybamm/simulation.py

@@ -790,14 +790,19 @@ def solve(
                     # human-intuitive
                     op_conds = self.experiment.operating_conditions_steps[idx]
 
+                    # Hacky patch to allow correct processing of end_time and next_starting time
+                    # For efficiency purposes, op_conds treats identical steps as the same object
+                    # regardless of the initial time. Should be refactored as part of #3176
+                    op_conds_unproc = self.experiment.operating_conditions_steps_unprocessed[idx]
+
                     start_time = current_solution.t[-1]
 
                     # If step has an end time, dt must take that into account
-                    if op_conds.end_time:
+                    if getattr(op_conds_unproc, "end_time", None):
                         dt = min(
                             op_conds.duration,
                             (
-                                op_conds.end_time
+                                op_conds_unproc.end_time
                                 - (
                                     initial_start_time
                                     + timedelta(seconds=float(start_time))
@@ -850,9 +855,9 @@ def solve(
                     step_termination = step_solution.termination
 
                     # Add a padding rest step if necessary
-                    if op_conds.next_start_time is not None:
+                    if getattr(op_conds_unproc, "next_start_time", None) is not None:
                         rest_time = (
-                            op_conds.next_start_time
+                            op_conds_unproc.next_start_time
                             - (
                                 initial_start_time
                                 + timedelta(seconds=float(step_solution.t[-1]))

tests/unit/test_experiments/test_experiment.py

@@ -183,41 +183,49 @@ def test_no_initial_start_time(self):
             )
 
     def test_set_next_start_time(self):
-        # Defined dummy experiment to access _set_next_start_time
-        experiment = pybamm.Experiment(["Rest for 1 hour"])
         raw_op = [
             pybamm.step._Step(
                 "current", 1, duration=3600, start_time=datetime(2023, 1, 1, 8, 0)
             ),
+            pybamm.step._Step("voltage", 2.5, duration=3600, start_time=None),
             pybamm.step._Step(
                 "current", 1, duration=3600, start_time=datetime(2023, 1, 1, 12, 0)
             ),
             pybamm.step._Step("current", 1, duration=3600, start_time=None),
+            pybamm.step._Step("voltage", 2.5, duration=3600, start_time=None),
             pybamm.step._Step(
                 "current", 1, duration=3600, start_time=datetime(2023, 1, 1, 15, 0)
             ),
         ]
+        experiment = pybamm.Experiment(raw_op)
         processed_op = experiment._set_next_start_time(raw_op)
 
         expected_next = [
+            None,
             datetime(2023, 1, 1, 12, 0),
             None,
+            None,
             datetime(2023, 1, 1, 15, 0),
             None,
         ]
 
         expected_end = [
             datetime(2023, 1, 1, 12, 0),
+            datetime(2023, 1, 1, 12, 0),
+            datetime(2023, 1, 1, 15, 0),
             datetime(2023, 1, 1, 15, 0),
             datetime(2023, 1, 1, 15, 0),
             None,
         ]
 
+        # Test method directly
         for next, end, op in zip(expected_next, expected_end, processed_op):
             # useful form for debugging
             self.assertEqual(op.next_start_time, next)
             self.assertEqual(op.end_time, end)
 
+        # TODO: once #3176 is completed, the test should pass for
+        # operating_conditions_steps (or equivalent) as well
 
 if __name__ == "__main__":
     print("Add -v for more debug output")