Three ways a mistyped config reaches something worse than an error

These are on the other side of the line from the ones taken out last commit.
Nothing here needs an unusual number, only a config file with a typo in it, and
two of the three say nothing at all when it happens.

`actuator_range` was read by index, so its shape was never checked. Three
endpoints dropped the third in silence, one raised IndexError, a bare float
raised TypeError, and string endpoints, which is what a YAML value without
quotes handling looks like, failed inside np.clip with a ufunc loop error. It
is now a pair of numbers or a ValueError naming the endpoint. Lists and numpy
scalars still work, and an infinite bound still means unbounded.

Each `add_*_control` removed the existing controller before building its
replacement. A second call with a rejected argument then left the rocket
carrying an actuator that simulation would never call, with no error after the
one that caused it: measured, `sampling_rate=-1` on a second call took the
controller list from one entry to zero while `throttle_control` stayed set. Both
halves are built first now.

`demand_rate=None` with a rate limit or a time constant was accepted with a
warning and then ignored. Asking for a 0.1 s lag and getting an instant response
is worth stopping for, and the subclass docstrings already say a demand rate is
required for those options. It raises at construction now.

Four mutations, one per fix, each fail a named test. 122 tests, up from 104.
pylint exits 0: rocket.py was one line over its 3050 limit after this, so the
comments came down to two lines each.

Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com>

Author: thc1006

rocketpy/rocket/actuator/actuator.py

@@ -47,6 +47,22 @@ def _non_negative_or_none(value, description):
     return number
 
 
+def _two_numbers_or_raise(value, description):
+    """A pair of numbers. Infinite is allowed here: it means unbounded."""
+    try:
+        lower, upper = value
+    except (TypeError, ValueError) as error:
+        raise ValueError(f"{description} must be exactly two numbers.") from error
+    numbers = []
+    for endpoint, where in ((lower, 0), (upper, 1)):
+        # Not float(), which takes "0" and would leave a YAML string range half
+        # converted while the rest of the config kept its strings.
+        if isinstance(endpoint, bool) or not isinstance(endpoint, (int, float)):
+            raise ValueError(f"{description}[{where}] must be a number.")
+        numbers.append(float(endpoint))
+    return numbers[0], numbers[1]
+
+
 class Actuator(ABC):
     """Abstract class used to define actuators.
 
@@ -105,9 +121,14 @@ def __init__(
         # default really is (-inf, inf), meaning an actuator with no range.
         self.demand_rate = _positive_or_none(demand_rate, "demand_rate")
 
-        if not actuator_range[0] <= actuator_range[1]:
+        # A range read out of a config file arrives as anything. Without the
+        # shape and number checks, three endpoints drop the third in silence and
+        # string endpoints fail inside np.clip with a ufunc loop error.
+        lower, upper = _two_numbers_or_raise(actuator_range, "actuator_range")
+        if not lower <= upper:
             raise ValueError("actuator_range[0] must be <= actuator_range[1].")
-        self.actuator_range = actuator_range
+        self.actuator_range = (lower, upper)
+        actuator_range = self.actuator_range
 
         self.actuator_rate_limit = _non_negative_or_none(
             actuator_rate_limit, "actuator_rate_limit"
@@ -118,6 +139,21 @@ def __init__(
         self.actuator_time_constant = _non_negative_or_none(
             actuator_time_constant, "actuator_time_constant"
         )
+
+        # Neither is implemented for a continuous actuator, and both were
+        # accepted with a warning. Asking for a 0.1 s lag and getting an
+        # instant response is worth stopping for rather than mentioning.
+        if self.demand_rate is None:
+            for option, given in (
+                ("actuator_rate_limit", self.actuator_rate_limit),
+                ("actuator_time_constant", self.actuator_time_constant),
+            ):
+                if given:
+                    raise ValueError(
+                        f"{option} needs a demand_rate: it is not applied to a "
+                        f"continuous actuator."
+                    )
+
         self._update_iir_coefficients()
 
         # An initial output outside the range used to survive here and come back

rocketpy/rocket/rocket.py

@@ -2075,13 +2075,6 @@ def add_thrust_vector_control(
                 "Only one thrust_vector_control per rocket is currently supported. "
                 + "Overwriting previous thrust_vector_control and controllers."
             )
-            self._controllers = [
-                controller
-                for controller in self._controllers
-                if not isinstance(
-                    controller.interactive_objects, ThrustVectorActuator2D
-                )
-            ]
 
         thrust_vector_control = ThrustVectorActuator2D(
             name=name,
@@ -2113,6 +2106,13 @@ def add_thrust_vector_control(
             initial_observed_variables=initial_observed_variables,
             name=controller_name,
         )
+        # Removed only once both halves are built, so a rejected argument on a
+        # second call leaves the rocket as it was.
+        self._controllers = [
+            controller
+            for controller in self._controllers
+            if not isinstance(controller.interactive_objects, ThrustVectorActuator2D)
+        ]
         self.thrust_vector_control = thrust_vector_control
         self._add_controllers(_controller)
         if return_controller:
@@ -2224,11 +2224,6 @@ def add_roll_control(
                 "Only one roll control per rocket is currently supported. "
                 + "Overwriting previous roll control and controllers."
             )
-            self._controllers = [
-                controller
-                for controller in self._controllers
-                if not isinstance(controller.interactive_objects, RollActuator)
-            ]
 
         roll_control = RollActuator(
             name=name,
@@ -2247,6 +2242,13 @@ def add_roll_control(
             initial_observed_variables=initial_observed_variables,
             name=controller_name,
         )
+        # Removed only once both halves are built, so a rejected argument on a
+        # second call leaves the rocket as it was.
+        self._controllers = [
+            controller
+            for controller in self._controllers
+            if not isinstance(controller.interactive_objects, RollActuator)
+        ]
         self.roll_control = roll_control
         self._add_controllers(_controller)
         if return_controller:
@@ -2360,11 +2362,6 @@ def add_throttle_control(
                 "Only one throttle control per rocket is currently supported. "
                 + "Overwriting previous throttle control and controllers."
             )
-            self._controllers = [
-                controller
-                for controller in self._controllers
-                if not isinstance(controller.interactive_objects, ThrottleActuator)
-            ]
 
         throttle_control = ThrottleActuator(
             name=name,
@@ -2385,6 +2382,13 @@ def add_throttle_control(
             name=controller_name,
         )
 
+        # Removed only once both halves are built, so a rejected argument on a
+        # second call leaves the rocket as it was.
+        self._controllers = [
+            controller
+            for controller in self._controllers
+            if not isinstance(controller.interactive_objects, ThrottleActuator)
+        ]
         self.throttle_control = throttle_control
         self._add_controllers(_controller)
 

tests/unit/rocket/test_actuators.py

@@ -721,6 +721,43 @@ def test_the_two_forms_agree_where_both_work(self, demand_rate, time_constant):
         )
 
 
+class TestAMisspeltRange:
+    """A range read out of a config file arrives as anything.
+
+    Three endpoints dropped the third in silence, one raised IndexError, and
+    string endpoints failed inside np.clip with a ufunc loop error.
+    """
+
+    @pytest.mark.parametrize(
+        "limits", [(0.0,), (0.0, 1.0, 2.0), ("0", "1"), 1.0, None, (True, False)]
+    )
+    def test_it_is_refused_at_the_boundary(self, limits):
+        with pytest.raises(ValueError, match="actuator_range"):
+            ThrottleActuator(throttle_range=limits)
+
+    @pytest.mark.parametrize("limits", [(0.0, 1.0), (0, 1), [0.0, 1.0]])
+    def test_the_spellings_that_work_still_work(self, limits):
+        assert ThrottleActuator(throttle_range=limits).actuator_range == (0.0, 1.0)
+
+
+class TestDynamicsNeedADemandRate:
+    """Neither is implemented for a continuous actuator.
+
+    Both were accepted with a warning, so asking for a 0.1 s lag gave an instant
+    response and said so in a line that a simulation log buries.
+    """
+
+    @pytest.mark.parametrize(
+        "option", [{"throttle_time_constant": 0.1}, {"throttle_rate_limit": 0.5}]
+    )
+    def test_asking_for_one_without_a_rate_is_refused(self, option):
+        with pytest.raises(ValueError, match="needs a demand_rate"):
+            ThrottleActuator(demand_rate=None, **option)
+
+    def test_a_continuous_actuator_on_its_own_still_builds(self):
+        assert ThrottleActuator(demand_rate=None).demand_rate is None
+
+
 def _no_op_controller(time, sampling_rate, state, state_history, observed, interactive):  # pylint: disable=unused-argument
     """A controller that commands nothing, so these tests are about the wiring."""
     return None
@@ -787,3 +824,45 @@ def test_the_controller_rate_is_usable_as_a_time_step(self, calisto, adder, extr
         controller = calisto._controllers[-1]
 
         assert 1 / controller.sampling_rate == pytest.approx(0.01)
+
+
+class TestAFailedReplacementLeavesTheRocketAlone:
+    """The old controller used to go before the new one was built.
+
+    A rejected argument on a second call then left the rocket carrying an
+    actuator that simulation would never call, with nothing said.
+    """
+
+    ADDERS = [
+        ("add_roll_control", {"max_roll_torque": 10.0}),
+        ("add_throttle_control", {}),
+        ("add_thrust_vector_control", {"max_gimbal_angle": 5.0}),
+    ]
+
+    @pytest.mark.parametrize("adder, extra", ADDERS)
+    def test_a_rejected_second_call_keeps_the_first_controller(
+        self, calisto, adder, extra
+    ):
+        getattr(calisto, adder)(
+            controller_function=_no_op_controller, sampling_rate=100, **extra
+        )
+        before = list(calisto._controllers)
+
+        with pytest.raises(ValueError):
+            getattr(calisto, adder)(
+                controller_function=_no_op_controller, sampling_rate=-1, **extra
+            )
+
+        assert calisto._controllers == before
+
+    @pytest.mark.parametrize("adder, extra", ADDERS)
+    def test_an_accepted_second_call_still_replaces(self, calisto, adder, extra):
+        getattr(calisto, adder)(
+            controller_function=_no_op_controller, sampling_rate=100, **extra
+        )
+        getattr(calisto, adder)(
+            controller_function=_no_op_controller, sampling_rate=50, **extra
+        )
+
+        assert len(calisto._controllers) == 1
+        assert calisto._controllers[0].sampling_rate == 50.0