#1839 rename timestamp to start_time and allow only datetime (Valentin's comments)

brosaplanella · brosaplanella · commit f641985dcbae · 2023-06-16T17:34:10.000+01:00
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,7 +10,7 @@
 
 - Enable multithreading in IDAKLU solver ([#2947](https://github.com/pybamm-team/PyBaMM/pull/2947))
 - If a solution contains cycles and steps, the cycle number and step number are now saved when `solution.save_data()` is called ([#2931](https://github.com/pybamm-team/PyBaMM/pull/2931))
-- Experiments can now be given a timestamp to trigger when each step should start ([#2616](https://github.com/pybamm-team/PyBaMM/pull/2616)) 
+- Experiments can now be given a `start_time` to define when each step should be triggered ([#2616](https://github.com/pybamm-team/PyBaMM/pull/2616)) 
 
 ## Optimizations
 
diff --git a/examples/notebooks/experiments-start-time.ipynb b/examples/notebooks/experiments-start-time.ipynb
@@ -6,7 +6,7 @@
    "id": "regional-bedroom",
    "metadata": {},
    "source": [
-    "# Timestamped experiments"
+    "# Experiments with `start_time`"
    ]
   },
   {
@@ -112,7 +112,7 @@
    "id": "0fdfced4",
    "metadata": {},
    "source": [
-    "However, if we want to represent a realistic user case we might certain experiments to be run at a certain time instead, even if that means cutting short the previous step. In this case we can pass a timestamp as a keyword argument in the `pybamm.step.string` method. The timestamp can be passed as a `datetime.datetime` object or as a string in one of the supported formats for the date. Date can be passed either as `Day 1` or `2023-01-01`, and time can be passed either as `08:00:00` or `08:00`."
+    "However, if we want to represent a realistic user case we might certain experiments to be run at a certain time instead, even if that means cutting short the previous step. In this case we can pass a starting time as a keyword argument in the `pybamm.step.string` method. The `start_time` should be passed as a `datetime.datetime` object."
    ]
   },
   {
@@ -151,9 +151,9 @@
     "\n",
     "experiment = pybamm.Experiment(\n",
     "    [\n",
-    "        s(\"Discharge at 1C for 1 hour\", timestamp=\"Day 1 08:00:00\"),\n",
-    "        s(\"Charge at C/3 for 10 minutes\", timestamp=\"Day 1 08:30:00\"),\n",
-    "        s(\"Discharge at C/2 for 30 minutes\", timestamp=\"Day 1 09:00:00\"),\n",
+    "        s(\"Discharge at 1C for 1 hour\", start_time=\"Day 1 08:00:00\"),\n",
+    "        s(\"Charge at C/3 for 10 minutes\", start_time=\"Day 1 08:30:00\"),\n",
+    "        s(\"Discharge at C/2 for 30 minutes\", start_time=\"Day 1 09:00:00\"),\n",
     "        s(\"Rest for 1 hour\"),\n",
     "    ]\n",
     ")\n",
@@ -168,7 +168,7 @@
    "id": "edfa4c9f",
    "metadata": {},
    "source": [
-    "In the example above, we note that the first step (1C discharge) is cut short as the second step (C/3 charge) timestamp occurs before the end of the first step. On the other hand, an additional resting period is added after the second step as the third step (C/2 discharge) timestamp is 20 minutes later than the end of the second step. The final step does not have a timestamp so it is triggered immediately after the previous step. Note that if timestamps are used in an experiment, the first step should always have a timestamp, otherwise the solver will throw an error."
+    "In the example above, we note that the first step (1C discharge) is cut short as the second step (C/3 charge) start time occurs before the end of the first step. On the other hand, an additional resting period is added after the second step as the third step (C/2 discharge) start time is 20 minutes later than the end of the second step. The final step does not have a start time so it is triggered immediately after the previous step. Note that if the argument `start_time` is used in an experiment, the first step should always have a `start_time`, otherwise the solver will throw an error."
    ]
   },
   {
@@ -221,7 +221,7 @@
    "name": "python",
    "nbconvert_exporter": "python",
    "pygments_lexer": "ipython3",
-   "version": "3.9.16"
+   "version": "3.9.17"
   },
   "toc": {
    "base_numbering": 1,
diff --git a/pybamm/experiment/experiment.py b/pybamm/experiment/experiment.py
@@ -83,7 +83,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_timestamp(
+        operating_conditions_steps_unprocessed = self._set_next_start_time(
             [cond for cycle in operating_conditions_cycles for cond in cycle]
         )
 
@@ -104,15 +104,15 @@ def __init__(
             processed_steps[step] for step in operating_conditions_steps_unprocessed
         ]
 
-        self.initial_timestamp = self.operating_conditions_steps[0].timestamp
+        self.initial_start_time = self.operating_conditions_steps[0].start_time
 
         if (
-            self.operating_conditions_steps[0].end_timestamp is not None
-            and self.initial_timestamp is None
+            self.operating_conditions_steps[0].end_time is not None
+            and self.initial_start_time is None
         ):
             raise ValueError(
-                "When using timestamped experiments, the first step must have a "
-                "timestamp to define the initial time."
+                "When using experiments with `start_time`, the first step must have a "
+                "`start_time`."
             )
 
         self.termination_string = termination
@@ -195,12 +195,12 @@ def search_tag(self, tag):
 
         return cycles
 
-    def _set_next_timestamp(self, operating_conditions):
+    def _set_next_start_time(self, operating_conditions):
         if all(isinstance(i, str) for i in operating_conditions):
             return operating_conditions
 
-        end_timestamp = None
-        next_timestamp = None
+        end_time = None
+        next_start_time = None
 
         for op in reversed(operating_conditions):
             if isinstance(op, str):
@@ -210,11 +210,11 @@ def _set_next_timestamp(self, operating_conditions):
                     "Operating conditions should be strings or _Step objects"
                 )
 
-            op.next_timestamp = next_timestamp
-            op.end_timestamp = end_timestamp
+            op.next_start_time = next_start_time
+            op.end_time = end_time
 
-            next_timestamp = op.timestamp
-            if next_timestamp:
-                end_timestamp = next_timestamp
+            next_start_time = op.start_time
+            if next_start_time:
+                end_time = next_start_time
 
         return operating_conditions
diff --git a/pybamm/simulation.py b/pybamm/simulation.py
@@ -257,8 +257,8 @@ def set_up_and_parameterise_model_for_experiment(self):
             )
             self.experiment_unique_steps_to_model[repr(op)] = parameterised_model
 
-        # Set up rest model if experiment has timestamps
-        if self.experiment.initial_timestamp:
+        # Set up rest model if experiment has start times
+        if self.experiment.initial_start_time:
             new_model = self.model.new_copy()
             # Update parameter values
             new_parameter_values = self.parameter_values.copy()
@@ -693,14 +693,14 @@ def solve(
 
                     start_time = current_solution.t[-1]
 
-                    # If step has an end timestamp, dt must take that into account
-                    if op_conds.end_timestamp:
+                    # If step has an end time, dt must take that into account
+                    if op_conds.end_time:
                         dt = min(
                             op_conds.duration,
                             (
-                                op_conds.end_timestamp
+                                op_conds.end_time
                                 - (
-                                    self.experiment.initial_timestamp
+                                    self.experiment.initial_start_time
                                     + timedelta(seconds=float(start_time))
                                 )
                             ).total_seconds(),
@@ -751,11 +751,11 @@ def solve(
                     step_termination = step_solution.termination
 
                     # Add a padding rest step if necessary
-                    if op_conds.next_timestamp is not None:
+                    if op_conds.next_start_time is not None:
                         rest_time = (
-                            op_conds.next_timestamp
+                            op_conds.next_start_time
                             - (
-                                self.experiment.initial_timestamp
+                                self.experiment.initial_start_time
                                 + timedelta(seconds=float(step_solution.t[-1]))
                             )
                         ).total_seconds()
diff --git a/pybamm/step/_steps_util.py b/pybamm/step/_steps_util.py
@@ -66,7 +66,7 @@ def __init__(
         period=None,
         temperature=None,
         tags=None,
-        timestamp=None,
+        start_time=None,
         description=None,
     ):
         self.type = typ
@@ -83,8 +83,8 @@ def __init__(
             self.args += f", temperature={temperature}"
         if tags:
             self.args += f", tags={tags}"
-        if timestamp:
-            self.args += f", timestamp={timestamp}"
+        if start_time:
+            self.args += f", start_time={start_time}"
         if description:
             self.args += f", description={description}"
 
@@ -144,9 +144,12 @@ def __init__(
             tags = [tags]
         self.tags = tags
 
-        self.timestamp = _process_timestamp(timestamp)
-        self.next_timestamp = None
-        self.end_timestamp = None
+        if start_time is None or isinstance(start_time, datetime):
+            self.start_time = start_time
+        else:
+            raise TypeError("`start_time` should be a datetime.datetime object")
+        self.next_start_time = None
+        self.end_time = None
 
     def __str__(self):
         if self.description is not None:
@@ -174,16 +177,16 @@ def to_dict(self):
             "period": self.period,
             "temperature": self.temperature,
             "tags": self.tags,
-            "timestamp": self.timestamp,
+            "start_time": self.start_time,
             "description": self.description,
         }
 
     def __eq__(self, other):
         return (
             isinstance(other, _Step)
             and self.__repr__() == other.__repr__()
-            and self.next_timestamp == other.next_timestamp
-            and self.end_timestamp == other.end_timestamp
+            and self.next_start_time == other.next_start_time
+            and self.end_time == other.end_time
         )
 
     def __hash__(self):
@@ -273,27 +276,3 @@ def _convert_electric(value_string):
             f"units must be 'A', 'V', 'W', 'Ohm', or 'C'. For example: {_examples}"
         )
     return typ, value
-
-
-datetime_formats = [
-    "Day %j %H:%M:%S",
-    "Day %j %H:%M",
-    "%Y-%m-%d %H:%M:%S",
-    "%Y-%m-%d %H:%M",
-]
-
-
-def _process_timestamp(timestamp):
-    if timestamp is None or isinstance(timestamp, datetime):
-        return timestamp
-    else:
-        for format in datetime_formats:
-            try:
-                return datetime.strptime(timestamp, format)
-            except ValueError:
-                pass
-
-        raise ValueError(
-            f"The timestamp [{timestamp}] does not match any "
-            "of the supported formats."
-        )
diff --git a/tests/unit/test_experiments/test_experiment.py b/tests/unit/test_experiments/test_experiment.py
@@ -28,7 +28,7 @@ def test_cycle_unpacking(self):
                     "description": "Discharge at C/20 for 0.5 hours",
                     "termination": [],
                     "tags": [],
-                    "timestamp": None,
+                    "start_time": None,
                 },
                 {
                     "value": -0.2,
@@ -39,7 +39,7 @@ def test_cycle_unpacking(self):
                     "description": "Charge at C/5 for 45 minutes",
                     "termination": [],
                     "tags": [],
-                    "timestamp": None,
+                    "start_time": None,
                 },
                 {
                     "value": 0.05,
@@ -50,7 +50,7 @@ def test_cycle_unpacking(self):
                     "description": "Discharge at C/20 for 0.5 hours",
                     "termination": [],
                     "tags": [],
-                    "timestamp": None,
+                    "start_time": None,
                 },
                 {
                     "value": -0.2,
@@ -61,7 +61,7 @@ def test_cycle_unpacking(self):
                     "description": "Charge at C/5 for 45 minutes",
                     "termination": [],
                     "tags": [],
-                    "timestamp": None,
+                    "start_time": None,
                 },
             ],
         )
@@ -172,29 +172,32 @@ def test_search_tag(self):
         self.assertEqual(experiment.search_tag("tag5"), [3])
         self.assertEqual(experiment.search_tag("no_tag"), [])
 
-    def test_no_initial_timestamp(self):
+    def test_no_initial_start_time(self):
         s = pybamm.step.string
-        with self.assertRaisesRegex(ValueError, "first step must have a timestamp"):
+        with self.assertRaisesRegex(ValueError, "first step must have a `start_time`"):
             pybamm.Experiment(
-                [s("Rest for 1 hour"), s("Rest for 1 hour", timestamp="Day 1 08:01:05")]
+                [
+                    s("Rest for 1 hour"),
+                    s("Rest for 1 hour", start_time=datetime(2023, 1, 1, 8, 0)),
+                ]
             )
 
-    def test_set_next_timestamp(self):
-        # Defined dummy experiment to access _set_next_timestamp
+    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, timestamp=datetime(2023, 1, 1, 8, 0)
+                "current", 1, duration=3600, start_time=datetime(2023, 1, 1, 8, 0)
             ),
             pybamm.step._Step(
-                "current", 1, duration=3600, timestamp=datetime(2023, 1, 1, 12, 0)
+                "current", 1, duration=3600, start_time=datetime(2023, 1, 1, 12, 0)
             ),
-            pybamm.step._Step("current", 1, duration=3600, timestamp=None),
+            pybamm.step._Step("current", 1, duration=3600, start_time=None),
             pybamm.step._Step(
-                "current", 1, duration=3600, timestamp=datetime(2023, 1, 1, 15, 0)
+                "current", 1, duration=3600, start_time=datetime(2023, 1, 1, 15, 0)
             ),
         ]
-        processed_op = experiment._set_next_timestamp(raw_op)
+        processed_op = experiment._set_next_start_time(raw_op)
 
         expected_next = [
             datetime(2023, 1, 1, 12, 0),
@@ -212,8 +215,8 @@ def test_set_next_timestamp(self):
 
         for next, end, op in zip(expected_next, expected_end, processed_op):
             # useful form for debugging
-            self.assertEqual(op.next_timestamp, next)
-            self.assertEqual(op.end_timestamp, end)
+            self.assertEqual(op.next_start_time, next)
+            self.assertEqual(op.end_time, end)
 
 
 if __name__ == "__main__":
diff --git a/tests/unit/test_experiments/test_experiment_steps.py b/tests/unit/test_experiments/test_experiment_steps.py
diff --git a/tests/unit/test_experiments/test_simulation_with_experiment.py b/tests/unit/test_experiments/test_simulation_with_experiment.py
