From d21a7b5ead4435c743ac062b5f02dd0b9a077116 Mon Sep 17 00:00:00 2001 From: Ferran Brosa Planella Date: Fri, 10 Nov 2023 11:15:09 +0000 Subject: [PATCH 1/7] fix bug with identical steps with different end times --- pybamm/experiment/experiment.py | 6 +++--- pybamm/simulation.py | 20 ++++++++++++++----- .../unit/test_experiments/test_experiment.py | 12 +++++++++-- 3 files changed, 28 insertions(+), 10 deletions(-) diff --git a/pybamm/experiment/experiment.py b/pybamm/experiment/experiment.py index d1c45015b6..9b02e3a20f 100644 --- a/pybamm/experiment/experiment.py +++ b/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 diff --git a/pybamm/simulation.py b/pybamm/simulation.py index 380105d215..9147a9a541 100644 --- a/pybamm/simulation.py +++ b/pybamm/simulation.py @@ -169,8 +169,10 @@ def set_up_and_parameterise_experiment(self): # Update experiment using capacity capacity = self._parameter_values["Nominal cell capacity [A.h]"] for op_conds in self.experiment.operating_conditions_steps: + print(op_conds.type) if op_conds.type == "C-rate": op_conds.type = "current" + print(" ", op_conds.type) op_conds.value = op_conds.value * capacity # Update terminations @@ -207,10 +209,13 @@ def set_up_and_parameterise_model_for_experiment(self): reduces simulation time since the model formulation is efficient. """ self.experiment_unique_steps_to_model = {} - for op_number, op in enumerate(self.experiment.unique_steps): + for op_number, op in enumerate(set(self.experiment.operating_conditions_steps)): new_model = self._model.new_copy() new_parameter_values = self._parameter_values.copy() + if op.type == "C-rate": + print(op) + if op.type != "current": # Voltage or power control # Create a new model where the current density is now a variable @@ -769,14 +774,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 op_conds_unproc.end_time: dt = min( op_conds.duration, ( - op_conds.end_time + op_conds_unproc.end_time - ( initial_start_time + timedelta(seconds=float(start_time)) @@ -829,9 +839,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 op_conds_unproc.next_start_time 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])) diff --git a/tests/unit/test_experiments/test_experiment.py b/tests/unit/test_experiments/test_experiment.py index 23548be433..ec1a1cbeae 100644 --- a/tests/unit/test_experiments/test_experiment.py +++ b/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") From 25278465cb60d182cc92226a6f48de0471b6f662 Mon Sep 17 00:00:00 2001 From: Ferran Brosa Planella Date: Fri, 10 Nov 2023 11:15:50 +0000 Subject: [PATCH 2/7] add copy method for steps --- pybamm/step/_steps_util.py | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/pybamm/step/_steps_util.py b/pybamm/step/_steps_util.py index e524bc6064..888863f9c7 100644 --- a/pybamm/step/_steps_util.py +++ b/pybamm/step/_steps_util.py @@ -70,6 +70,7 @@ def __init__( description=None, ): self.type = typ + self.raw_termination = termination # Record all the args for repr and hash self.repr_args = f"{typ}, {value}" @@ -171,6 +172,19 @@ def basic_repr(self): """ return f"_Step({self.hash_args})" + def copy(self): + return _Step( + typ=self.type, + value=self.value, + duration=self.duration, + termination=self.raw_termination, + period=self.period, + temperature=self.temperature, + tags=self.tags, + start_time=self.start_time, + description=self.description, + ) + def to_dict(self): """ Convert the step to a dictionary. From 2d0f81c117acc370d703e8a5a8b112696b2e6ca4 Mon Sep 17 00:00:00 2001 From: Ferran Brosa Planella Date: Fri, 10 Nov 2023 11:30:07 +0000 Subject: [PATCH 3/7] undo testing changes --- pybamm/simulation.py | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/pybamm/simulation.py b/pybamm/simulation.py index 9147a9a541..25b7a96902 100644 --- a/pybamm/simulation.py +++ b/pybamm/simulation.py @@ -169,10 +169,8 @@ def set_up_and_parameterise_experiment(self): # Update experiment using capacity capacity = self._parameter_values["Nominal cell capacity [A.h]"] for op_conds in self.experiment.operating_conditions_steps: - print(op_conds.type) if op_conds.type == "C-rate": op_conds.type = "current" - print(" ", op_conds.type) op_conds.value = op_conds.value * capacity # Update terminations @@ -209,13 +207,10 @@ def set_up_and_parameterise_model_for_experiment(self): reduces simulation time since the model formulation is efficient. """ self.experiment_unique_steps_to_model = {} - for op_number, op in enumerate(set(self.experiment.operating_conditions_steps)): + for op_number, op in enumerate(self.experiment.unique_steps): new_model = self._model.new_copy() new_parameter_values = self._parameter_values.copy() - if op.type == "C-rate": - print(op) - if op.type != "current": # Voltage or power control # Create a new model where the current density is now a variable From 0660d9c8362ab51c4fba199487883d26d80c8175 Mon Sep 17 00:00:00 2001 From: Ferran Brosa Planella Date: Fri, 10 Nov 2023 12:06:43 +0000 Subject: [PATCH 4/7] fix failing tests --- pybamm/simulation.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/pybamm/simulation.py b/pybamm/simulation.py index 25b7a96902..927fa8dd3b 100644 --- a/pybamm/simulation.py +++ b/pybamm/simulation.py @@ -777,7 +777,7 @@ def solve( start_time = current_solution.t[-1] # If step has an end time, dt must take that into account - if op_conds_unproc.end_time: + if getattr(op_conds_unproc, "end_time", None): dt = min( op_conds.duration, ( @@ -834,7 +834,7 @@ def solve( step_termination = step_solution.termination # Add a padding rest step if necessary - if op_conds_unproc.next_start_time is not None: + if getattr(op_conds_unproc, "next_start_time", None) is not None: rest_time = ( op_conds_unproc.next_start_time - ( From b3cb203b7710fd5472cd7ecd0604387e331b3485 Mon Sep 17 00:00:00 2001 From: Ferran Brosa Planella Date: Wed, 15 Nov 2023 12:37:46 +0000 Subject: [PATCH 5/7] update CHANGELOG --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index afbc5073b0..75b114c967 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ ## Bug fixes +- Fixed bug that made identical Experiment steps with different end times crash ([#3516](https://github.com/pybamm-team/PyBaMM/pull/3516)) - Fixed bug in calculation of theoretical energy that made it very slow ([#3506](https://github.com/pybamm-team/PyBaMM/pull/3506)) # [v23.9rc0](https://github.com/pybamm-team/PyBaMM/tree/v23.9rc0) - 2023-10-31 From f07294ba0fa45232f41b6b283886ed6bcb703827 Mon Sep 17 00:00:00 2001 From: Ferran Brosa Planella Date: Wed, 15 Nov 2023 13:01:12 +0000 Subject: [PATCH 6/7] remove copy method as it is unused --- pybamm/step/_steps_util.py | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/pybamm/step/_steps_util.py b/pybamm/step/_steps_util.py index 888863f9c7..114aff5e98 100644 --- a/pybamm/step/_steps_util.py +++ b/pybamm/step/_steps_util.py @@ -172,19 +172,6 @@ def basic_repr(self): """ return f"_Step({self.hash_args})" - def copy(self): - return _Step( - typ=self.type, - value=self.value, - duration=self.duration, - termination=self.raw_termination, - period=self.period, - temperature=self.temperature, - tags=self.tags, - start_time=self.start_time, - description=self.description, - ) - def to_dict(self): """ Convert the step to a dictionary. From 05985415b0fe754f9df7ea2cc8fe940f8892f3ef Mon Sep 17 00:00:00 2001 From: Ferran Brosa Planella Date: Wed, 15 Nov 2023 13:02:06 +0000 Subject: [PATCH 7/7] remove raw termination as unused --- pybamm/step/_steps_util.py | 1 - 1 file changed, 1 deletion(-) diff --git a/pybamm/step/_steps_util.py b/pybamm/step/_steps_util.py index 114aff5e98..e524bc6064 100644 --- a/pybamm/step/_steps_util.py +++ b/pybamm/step/_steps_util.py @@ -70,7 +70,6 @@ def __init__( description=None, ): self.type = typ - self.raw_termination = termination # Record all the args for repr and hash self.repr_args = f"{typ}, {value}"