Reject NaN in the actuator argument checks

The checks in the previous commit read as the inverted comparison, and
inverting is not the same as negating once NaN is in play. Every ordered
comparison against NaN is false, so `nan <= 0` is false and the value went
through, where the assert being replaced asked `nan > 0` and rejected it.
Four arguments regressed that way: demand_rate, actuator_range,
actuator_rate_limit and actuator_time_constant.

Negate the positive predicate instead, which keeps the assert's meaning
exactly and leaves normal values alone.

Refuse a NaN initial output for the same reason. np.clip returns NaN for
NaN, so clamping would have stored it and _reset() would have restored it,
and there is no direction to clamp it towards in any case.

Drive both the in-process and the -O test from one table of invalid
arguments, so an argument cannot be rejected in the default interpreter
and accepted under -O. Each entry names the message it expects, so a case
cannot pass on some other argument's check. Add the zero cases on both
sides: demand_rate must refuse it, and the non-negative bounds must accept
it, since that is the documented no-dynamics and no-rate-limit setting.

Give the out-of-range warning a stacklevel so it is not reported against
the same line in actuator.py for every caller, and say in the three public
Rocket docstrings what the constructors now enforce.

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

Author: thc1006

rocketpy/rocket/actuator/actuator.py

@@ -52,21 +52,25 @@ def __init__(
         # These are argument checks rather than internal invariants, so they raise
         # instead of asserting: python -O drops assert statements, and a negative
         # time constant or rate limit would then be accepted in silence.
-        if demand_rate is not None and demand_rate <= 0:
+        # Negating the positive predicate rather than inverting the comparison.
+        # Every ordered comparison against NaN is false, so `nan <= 0` is false
+        # and would have let it through, where the assert this replaces asked
+        # `nan > 0` and rejected it. Same for the three below.
+        if demand_rate is not None and not demand_rate > 0:
             raise ValueError("demand_rate must be positive or None.")
         self.demand_rate = demand_rate
 
-        if actuator_range[0] > actuator_range[1]:
+        if not actuator_range[0] <= actuator_range[1]:
             raise ValueError("actuator_range[0] must be <= actuator_range[1].")
         self.actuator_range = actuator_range
 
-        if actuator_rate_limit is not None and actuator_rate_limit < 0:
+        if actuator_rate_limit is not None and not actuator_rate_limit >= 0:
             raise ValueError("actuator_rate_limit must be non-negative or None.")
         self.actuator_rate_limit = actuator_rate_limit
 
         self.clamp = clamp
 
-        if actuator_time_constant is not None and actuator_time_constant < 0:
+        if actuator_time_constant is not None and not actuator_time_constant >= 0:
             raise ValueError("actuator_time_constant must be non-negative or None.")
         self.actuator_time_constant = actuator_time_constant
         self._update_iir_coefficients()
@@ -75,14 +79,20 @@ def __init__(
         # on every _reset(), even though the output setter would never let the
         # actuator reach such a value afterwards. Treat it the way the setter
         # treats any other out-of-range value, so the two agree.
+        # NaN first: np.clip propagates it, so clamping would store NaN and
+        # _reset() would restore it, and there is no direction to clamp it
+        # towards in any case.
+        if not actuator_initial_output == actuator_initial_output:
+            raise ValueError(f"Actuator '{name}' initial output must be a number.")
         if self.clamp:
             actuator_initial_output = float(
                 np.clip(actuator_initial_output, actuator_range[0], actuator_range[1])
             )
         elif not actuator_range[0] <= actuator_initial_output <= actuator_range[1]:
             warnings.warn(
                 f"Actuator '{name}' initial output {actuator_initial_output} "
-                f"is outside its range {actuator_range}."
+                f"is outside its range {actuator_range}.",
+                stacklevel=2,
             )
 
         self.actuator_initial_output = actuator_initial_output

rocketpy/rocket/rocket.py

@@ -2017,14 +2017,16 @@ def add_thrust_vector_control(
         sampling_rate : float
             The sampling rate of the controller function in Hertz (Hz). This
             means that the controller function will be called every
-            `1/sampling_rate` seconds.
+            `1/sampling_rate` seconds. Must be positive, or None for a
+            continuous-time controller called at every integration step.
         max_gimbal_angle : int, float
             Maximum gimbal angle in degrees. Both x and y gimbal
             angles are clamped to this range if clamp is True. Must be
             non-negative.
         gimbal_rate_limit : int, float
             Maximum gimbal rate in degrees per second. Both x and y gimbal
-            angles are limited to this rate of change. Default is None, no rate limit.
+            angles are limited to this rate of change. Must be non-negative.
+            Default is None, no rate limit.
         clamp : bool, optional
             If True, the simulation will clamp gimbal angles to the range
             [-max_gimbal_angle, max_gimbal_angle]. If False, a warning is
@@ -2033,10 +2035,12 @@ def add_thrust_vector_control(
             The initial gimbal angle in degrees. If a single value is provided,
             it is used for both x and y gimbal angles. If a tuple or list is
             provided, the first element is used for the x-axis and the second
-            for the y-axis. Default is 0.0.
+            for the y-axis. A value outside the range is clamped into it when
+            clamp is True, and warned about otherwise. Default is 0.0.
         gimbal_time_constant : float, optional
-            Time constant for the gimbal dynamics in seconds. If None, no
-            gimbal dynamics are applied. Default is None.
+            Time constant for the gimbal dynamics in seconds. Must be
+            non-negative. If None, no gimbal dynamics are applied. Default is
+            None.
         initial_observed_variables : list, optional
             A list of the initial values of the variables that the controller
             function manages. This list is used to initialize the
@@ -2152,7 +2156,8 @@ def add_roll_control(
         sampling_rate : float
             The sampling rate of the controller function in Hertz (Hz). This
             means that the controller function will be called every
-            `1/sampling_rate` seconds.
+            `1/sampling_rate` seconds. Must be positive, or None for a
+            continuous-time controller called at every integration step.
         max_roll_torque : int, float
             Maximum roll torque magnitude in N·m. Must be non-negative.
         torque_rate_limit : int, float
@@ -2163,9 +2168,12 @@ def add_roll_control(
             [-max_roll_torque, max_roll_torque]. If False, a warning is
             issued when roll torque exceeds the range. Default is True.
         initial_roll_torque : int, float
-            Initial roll torque in N·m. Default is 0.0.
+            Initial roll torque in N·m. A value outside the range is clamped
+            into it when clamp is True, and warned about otherwise. Default is
+            0.0.
         roll_torque_time_constant : float, optional
-            Time constant for the roll torque dynamics in seconds. Default is None, no dynamics are applied.
+            Time constant for the roll torque dynamics in seconds. Must be
+            non-negative. Default is None, no dynamics are applied.
         initial_observed_variables : list, optional
             A list of the initial values of the variables that the controller
             function manages. This list is used to initialize the
@@ -2281,7 +2289,8 @@ def add_throttle_control(
         sampling_rate : float
             The sampling rate of the controller function in Hertz (Hz). This
             means that the controller function will be called every
-            `1/sampling_rate` seconds.
+            `1/sampling_rate` seconds. Must be positive, or None for a
+            continuous-time controller called at every integration step.
         throttle_range : tuple, optional
             A tuple containing the minimum and maximum throttle values. Must be in the range [0, 1]. Default is (0.0, 1.0).
         throttle_rate_limit : float, optional
@@ -2292,11 +2301,13 @@ def add_throttle_control(
             [throttle_range[0], throttle_range[1]]. If False, a warning is issued when
             throttle values exceed the range. Default is True.
         initial_throttle : float, optional
-            Initial throttle value at the start of the simulation. Must be within
-            the range [throttle_range[0], throttle_range[1]]. Default is 1.0.
+            Initial throttle value at the start of the simulation. A value
+            outside [throttle_range[0], throttle_range[1]] is clamped into the
+            range when clamp is True, and warned about otherwise. Default is
+            1.0.
         throttle_time_constant : float, optional
-            Time constant for the throttle actuator dynamics in seconds.
-            If None, no actuator dynamics are applied.
+            Time constant for the throttle actuator dynamics in seconds. Must be
+            non-negative. If None, no actuator dynamics are applied.
         initial_observed_variables : list, optional
             A list of the initial values of the variables that the controller
             function manages. This list is used to initialize the

tests/unit/rocket/test_actuators.py

@@ -1,3 +1,4 @@
+import math
 import subprocess
 import sys
 
@@ -455,60 +456,78 @@ def test_time_constant_iir_filter(self):
         assert initial_output < filtered_output < 1.0
 
 
-class TestActuatorValidation:
-    """Test suite for actuator parameter validation."""
-
-    def test_invalid_demand_rate_negative(self):
-        """Test that negative demand rate is rejected."""
-        with pytest.raises(ValueError):
-            RollActuator(demand_rate=-1)
+NAN = float("nan")
 
-    def test_invalid_range(self):
-        """Test that invalid range is rejected."""
-        with pytest.raises(ValueError):
-            RollActuator(
-                max_roll_torque=-5
-            )  # This creates range (5, -5) which is invalid
 
-    def test_invalid_time_constant_negative(self):
-        """Test that negative time constant is rejected."""
-        with pytest.raises(ValueError):
-            ThrottleActuator(throttle_time_constant=-0.1)
+def _as_source(value):
+    """Render a value as source the ``-O`` subprocess can evaluate.
 
-    def test_invalid_rate_limit_negative(self):
-        """Test that negative rate limit is rejected."""
-        with pytest.raises(ValueError):
-            ThrustVectorActuator(gimbal_rate_limit=-1.0)
+    ``repr`` is almost enough, except that it renders NaN as the bare name
+    ``nan``, which the subprocess does not have bound. Left as ``repr`` the NaN
+    cases died on NameError, and a test that only checked the return code would
+    have called that a pass.
+    """
+    if isinstance(value, float) and math.isnan(value):
+        return 'float("nan")'
+    if isinstance(value, tuple):
+        return "(" + ", ".join(_as_source(item) for item in value) + ",)"
+    return repr(value)
+
+
+# One table, walked twice: once in-process for the message, once under ``-O``.
+# Keeping them in step is the point. An argument that is only rejected in the
+# default interpreter is not rejected, because the checks these replaced were
+# asserts and asserts are what ``-O`` removes.
+#
+# Each entry names the message it expects, so a case cannot pass on some other
+# argument's check. Every NaN case is here because NaN fails every ordered
+# comparison: `nan <= 0` is false just as `nan > 0` is, so a check written as the
+# inverted comparison accepts it while the assert it replaces rejected it.
+INVALID_ARGUMENTS = [
+    (RollActuator, {"demand_rate": -1}, "demand_rate"),
+    (RollActuator, {"demand_rate": 0}, "demand_rate"),
+    (RollActuator, {"demand_rate": NAN}, "demand_rate"),
+    (RollActuator, {"max_roll_torque": -5}, "actuator_range"),
+    (ThrottleActuator, {"throttle_range": (NAN, 1.0)}, "actuator_range"),
+    (ThrottleActuator, {"throttle_range": (0.0, NAN)}, "actuator_range"),
+    (ThrustVectorActuator, {"gimbal_rate_limit": -1.0}, "rate_limit"),
+    (ThrustVectorActuator, {"gimbal_rate_limit": NAN}, "rate_limit"),
+    (ThrottleActuator, {"throttle_time_constant": -0.1}, "time_constant"),
+    (ThrottleActuator, {"throttle_time_constant": NAN}, "time_constant"),
+    (ThrottleActuator, {"initial_throttle": NAN}, "initial output"),
+    # clamp is what would otherwise absorb an out-of-range initial value, and
+    # np.clip returns NaN for NaN, so both settings have to refuse it.
+    (ThrottleActuator, {"initial_throttle": NAN, "clamp": False}, "initial output"),
+]
+INVALID_IDS = [
+    f"{cls.__name__}-{'-'.join(kwargs)}-{'clamped' if kwargs.get('clamp', True) else 'unclamped'}"
+    for cls, kwargs, _ in INVALID_ARGUMENTS
+]
 
-    def test_demand_rate_none_builds_a_continuous_actuator(self):
-        """None is the documented continuous-time mode and must be accepted.
 
-        The check used to read ``demand_rate > 0 or demand_rate is None``, and
-        Python evaluates the left operand first, so this raised TypeError and the
-        mode could not be constructed at all.
-        """
-        actuator = RollActuator(demand_rate=None)
+class TestActuatorValidation:
+    """Test suite for actuator parameter validation."""
 
-        assert actuator.demand_rate is None
+    @pytest.mark.parametrize(
+        "actuator_class, kwargs, message", INVALID_ARGUMENTS, ids=INVALID_IDS
+    )
+    def test_invalid_arguments_are_rejected(self, actuator_class, kwargs, message):
+        with pytest.raises(ValueError, match=message):
+            actuator_class(**kwargs)
 
     @pytest.mark.parametrize(
-        "actuator_class, kwargs",
-        [
-            (RollActuator, {"demand_rate": -1}),
-            (RollActuator, {"max_roll_torque": -5}),
-            (ThrottleActuator, {"throttle_time_constant": -0.1}),
-            (ThrustVectorActuator, {"gimbal_rate_limit": -1.0}),
-        ],
+        "actuator_class, kwargs, message", INVALID_ARGUMENTS, ids=INVALID_IDS
     )
-    def test_validation_survives_optimized_mode(self, actuator_class, kwargs):
+    def test_validation_survives_optimized_mode(self, actuator_class, kwargs, message):
         """``python -O`` drops assert statements, so these must not be asserts.
 
         Run in a subprocess because the flag is set at interpreter startup. Under
         the old bare asserts every one of these was accepted in silence.
         """
+        arguments = ", ".join(f"{k}={_as_source(v)}" for k, v in kwargs.items())
         source = (
             "from rocketpy.rocket.actuator import "
-            f"{actuator_class.__name__} as A; A(**{kwargs!r})"
+            f"{actuator_class.__name__} as A; A({arguments})"
         )
         result = subprocess.run(
             [sys.executable, "-O", "-c", source],
@@ -519,6 +538,41 @@ def test_validation_survives_optimized_mode(self, actuator_class, kwargs):
 
         assert result.returncode != 0, "invalid arguments were accepted under -O"
         assert "ValueError" in result.stderr
+        assert message in result.stderr
+
+    def test_demand_rate_none_builds_a_continuous_actuator(self):
+        """None is the documented continuous-time mode and must be accepted.
+
+        The check used to read ``demand_rate > 0 or demand_rate is None``, and
+        Python evaluates the left operand first, so this raised TypeError and the
+        mode could not be constructed at all.
+        """
+        actuator = RollActuator(demand_rate=None)
+
+        assert actuator.demand_rate is None
+
+    @pytest.mark.parametrize(
+        "actuator_class, kwargs",
+        [
+            (RollActuator, {"torque_rate_limit": 0.0}),
+            (ThrottleActuator, {"throttle_time_constant": 0.0}),
+            (ThrustVectorActuator, {"gimbal_rate_limit": 0.0}),
+        ],
+    )
+    def test_zero_is_accepted_where_the_bound_is_non_negative(
+        self, actuator_class, kwargs
+    ):
+        """Zero is on the legal side of every non-negative bound.
+
+        Worth its own case because the fix moved these from ``x < 0`` to
+        ``not x >= 0``, and an off-by-one there would turn the documented "no
+        dynamics" and "no rate limit" settings into errors. A zero rate limit
+        does freeze the actuator, but that is the caller's business, and the
+        constructor is not where that is decided.
+        """
+        actuator = actuator_class(**kwargs)
+
+        assert actuator is not None
 
 
 class TestActuatorInitialOutput:
@@ -557,6 +611,26 @@ def test_without_clamping_it_warns_instead(self):
 
         assert actuator.actuator_initial_output == 2.0
 
+    def test_the_warning_is_not_blamed_on_the_actuator_module(self):
+        """``pytest.warns`` reads the message, and the message is not the whole
+        warning. Without a stacklevel the report points at the ``warnings.warn``
+        line inside ``actuator.py``, which is the same line for every caller, so
+        ``-W`` filters keyed on a module and the printed location are both
+        useless.
+
+        Asserting the negative rather than a specific file because the warning is
+        raised in a base ``__init__`` reached through ``super()``, so no single
+        stacklevel lands on user code for every actuator: 2 reaches the concrete
+        subclass, and the dual-axis actuator adds another frame on top of that.
+        Not blaming the base module is the part that holds for all of them.
+        """
+        with pytest.warns(UserWarning, match="outside its range") as record:
+            ThrottleActuator(
+                throttle_range=(0.0, 1.0), initial_throttle=2.0, clamp=False
+            )
+
+        assert not record[0].filename.endswith("actuator.py")
+
 
 class TestActuatorWarnings:
     """Test suite for actuator warning conditions."""