Validate the value, not the reflexivity of a comparison

Pylint was red on this branch. The finding is one line, not the five
negated comparisons it looks like from the outside:

    C0117 unnecessary-negation
    R0124 comparison-with-itself
    not actuator_initial_output == actuator_initial_output

That is the NaN idiom, and it reads as a redundant comparison to a
reader and to pylint alike. Reverting it to `<=` would let NaN back in,
which is what the whole change is about, so it becomes a positive test
on the value instead.

Doing it that way settles two things the comparisons never named.
Infinity is refused where a finite number is required: a demand rate of
infinity makes the sampling period zero, which drives the IIR
coefficient to zero and freezes the output. And a value that is not a
number at all is refused there rather than a few lines later inside
np.clip. The range endpoints are deliberately left alone, since the base
default really is (-inf, inf), meaning an actuator with no range.

The same rule now guards the output setter, which is the more important
half. The constructor rejected a NaN and the setter accepted one on the
next timestep: np.clip returns NaN for NaN and both range comparisons
are false for NaN, so neither the clamped nor the unclamped branch
noticed and it was stored. That is the path an agent writes to.
BalloonPoppingChallenge assigns its actions straight into these setters,
and a policy that goes unstable emits NaN, which from there reaches the
forces, the moments and the integrator state.

Two smaller things found on the way. The -O subprocess helper rendered
NaN as source but not infinity, so the new cases would have died on
NameError while the test read the return code and called it a pass; it
covers both now. And the three checks are extracted, because inlining
them pushed __init__ over pylint's statement limit and suppressing that
would have been the wrong way round.

Also corrects a docstring that promised a feature nobody wrote.
Rocket.add_thrust_vector_control said initial_gimbal_angle could be a
per-axis tuple. ThrustVectorActuator2D hands its value to both
single-axis actuators unchanged and to_dict records only the x-axis
value, so an asymmetric pair could not survive a round trip either. It
used to be accepted and silently applied to both axes and now raises,
since the initial output is converted to a float. Corrected rather than
implemented: the feature belongs upstream rather than in this fork.

Verified by mutation: the setter accepting non-finite values, the check
narrowed back to NaN alone, and the demand rate bound dropped all fail.

pylint rocketpy/ tests/ docs/ rates 10.00/10, ruff check and format are
clean, and tests/unit/rocket passes 260.

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

Author: thc1006

rocketpy/rocket/actuator/actuator.py

@@ -4,6 +4,49 @@
 import numpy as np
 
 
+def _finite_or_raise(value, description):
+    """Return ``value`` as a float, refusing anything that is not finite.
+
+    A positive test on the value rather than a negated comparison. NaN fails
+    every ordered comparison, so a check written as ``value <= 0`` accepts it
+    where the assert it replaced rejected it; and the self-comparison that
+    caught it, ``not value == value``, reads as a redundant comparison to a
+    reader and to pylint alike.
+
+    This also settles two cases the comparisons never named. Infinity is
+    refused, since an actuator cannot start at or be driven to one, and a value
+    that is not a number at all is refused here rather than a few lines later
+    inside ``np.clip``.
+    """
+    try:
+        number = float(value)
+    except (TypeError, ValueError) as error:
+        raise ValueError(f"{description} must be a number.") from error
+    if not np.isfinite(number):
+        raise ValueError(f"{description} must be a finite number.")
+    return number
+
+
+def _positive_or_none(value, description):
+    """An optional number that has to be finite and greater than zero."""
+    if value is None:
+        return None
+    number = _finite_or_raise(value, description)
+    if number <= 0:
+        raise ValueError(f"{description} must be positive or None.")
+    return number
+
+
+def _non_negative_or_none(value, description):
+    """An optional number that has to be finite and not below zero."""
+    if value is None:
+        return None
+    number = _finite_or_raise(value, description)
+    if number < 0:
+        raise ValueError(f"{description} must be non-negative or None.")
+    return number
+
+
 class Actuator(ABC):
     """Abstract class used to define actuators.
 
@@ -52,38 +95,41 @@ 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.
-        # 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
+        # Finite first, then the bound, so each check says one thing. Written
+        # as a negated comparison these accepted NaN, which fails every ordered
+        # comparison, and infinity, which passes them: a demand rate of infinity
+        # makes the sampling period zero, which drives the IIR coefficient to
+        # zero and freezes the output.
+        #
+        # The range endpoints are deliberately not put through this. The base
+        # 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]:
             raise ValueError("actuator_range[0] must be <= actuator_range[1].")
         self.actuator_range = actuator_range
 
-        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.actuator_rate_limit = _non_negative_or_none(
+            actuator_rate_limit, "actuator_rate_limit"
+        )
 
         self.clamp = clamp
 
-        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.actuator_time_constant = _non_negative_or_none(
+            actuator_time_constant, "actuator_time_constant"
+        )
         self._update_iir_coefficients()
 
         # An initial output outside the range used to survive here and come back
         # 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.")
+        # Refused before clamping: np.clip propagates NaN, so clamping would
+        # store it and _reset() would restore it, and there is no direction to
+        # clamp it towards in any case.
+        actuator_initial_output = _finite_or_raise(
+            actuator_initial_output, f"Actuator '{name}' initial output"
+        )
         if self.clamp:
             actuator_initial_output = float(
                 np.clip(actuator_initial_output, actuator_range[0], actuator_range[1])
@@ -136,6 +182,18 @@ def actuator_output(self, value):
         -------
         None
         """
+        # The same rule as the initial output, so the two agree. It used to
+        # reject a NaN handed to the constructor and accept one handed to the
+        # setter on the next timestep: np.clip returns NaN for NaN, and both
+        # range comparisons are false for NaN, so neither branch noticed and it
+        # was stored.
+        #
+        # This is the path an agent writes to. BalloonPoppingChallenge assigns
+        # its actions straight into these setters, and a policy that goes
+        # unstable emits NaN, which from here reaches the forces, the moments
+        # and the integrator state.
+        value = _finite_or_raise(value, f"Actuator '{self.name}' output")
+
         # Apply first-order IIR actuator dynamics
         value = self._alpha * value + (1 - self._alpha) * self._actuator_output
 

rocketpy/rocket/rocket.py

@@ -2031,12 +2031,19 @@ def add_thrust_vector_control(
             If True, the simulation will clamp gimbal angles to the range
             [-max_gimbal_angle, max_gimbal_angle]. If False, a warning is
             issued when gimbal angles exceed the range. Default is True.
-        initial_gimbal_angle : int, float, tuple, list
-            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. A value outside the range is clamped into it when
-            clamp is True, and warned about otherwise. Default is 0.0.
+        initial_gimbal_angle : int, float
+            The initial gimbal angle in degrees, used for both the x and y
+            axes. A value outside the range is clamped into it when clamp is
+            True, and warned about otherwise. Default is 0.0.
+
+            A per-axis tuple or list was described here and has never been
+            implemented: ``ThrustVectorActuator2D`` hands the value it is given
+            to both single-axis actuators unchanged, and ``to_dict`` records
+            only the x-axis value, so an asymmetric pair could not survive a
+            round trip either. It used to be accepted and silently applied to
+            both axes; it now raises, because the initial output is converted
+            to a float. Corrected here rather than implemented, since the
+            feature belongs upstream rather than in this fork.
         gimbal_time_constant : float, optional
             Time constant for the gimbal dynamics in seconds. Must be
             non-negative. If None, no gimbal dynamics are applied. Default is

tests/unit/rocket/test_actuators.py

@@ -457,18 +457,19 @@ def test_time_constant_iir_filter(self):
 
 
 NAN = float("nan")
+INF = float("inf")
 
 
 def _as_source(value):
     """Render a value as source the ``-O`` subprocess can evaluate.
 
     ``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.
+    ``nan`` and infinity as ``inf``, neither of which the subprocess has bound.
+    Left as ``repr`` those 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, float) and not math.isfinite(value):
+        return f'float("{value}")'
     if isinstance(value, tuple):
         return "(" + ", ".join(_as_source(item) for item in value) + ",)"
     return repr(value)
@@ -498,13 +499,57 @@ def _as_source(value):
     # 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"),
+    # Infinity was never named by the comparisons. An actuator cannot start at
+    # one, and clamping would quietly turn it into a range endpoint.
+    (ThrottleActuator, {"initial_throttle": INF}, "initial output"),
+    (RollActuator, {"demand_rate": INF}, "demand_rate"),
 ]
 INVALID_IDS = [
     f"{cls.__name__}-{'-'.join(kwargs)}-{'clamped' if kwargs.get('clamp', True) else 'unclamped'}"
     for cls, kwargs, _ in INVALID_ARGUMENTS
 ]
 
 
+class TestTheOutputSetterRefusesWhatTheConstructorDoes:
+    """The two used to disagree, and the setter is the one an agent writes to.
+
+    A NaN handed to the constructor was rejected; the same NaN handed to the
+    setter on the next timestep was stored. ``np.clip`` returns NaN for NaN, and
+    both range comparisons are false for NaN, so neither the clamped nor the
+    unclamped branch noticed.
+
+    BalloonPoppingChallenge assigns agent actions straight into these setters,
+    and a policy that goes unstable emits NaN, which from there reaches the
+    forces, the moments and the integrator state.
+    """
+
+    @pytest.mark.parametrize("clamp", [True, False], ids=["clamped", "unclamped"])
+    @pytest.mark.parametrize("value", [NAN, INF, -INF], ids=["nan", "inf", "-inf"])
+    def test_a_non_finite_command_is_refused(self, clamp, value):
+        actuator = ThrottleActuator(clamp=clamp)
+
+        with pytest.raises(ValueError, match="output"):
+            actuator.actuator_output = value
+
+    @pytest.mark.parametrize("clamp", [True, False], ids=["clamped", "unclamped"])
+    def test_an_ordinary_command_still_goes_through(self, clamp):
+        """Or refusing everything would satisfy the test above."""
+        actuator = ThrottleActuator(clamp=clamp)
+
+        actuator.actuator_output = 0.5
+
+        assert actuator.actuator_output == pytest.approx(0.5)
+
+    def test_the_stored_output_is_untouched_by_a_refused_command(self):
+        actuator = ThrottleActuator()
+        actuator.actuator_output = 0.5
+
+        with pytest.raises(ValueError):
+            actuator.actuator_output = NAN
+
+        assert actuator.actuator_output == pytest.approx(0.5)
+
+
 class TestActuatorValidation:
     """Test suite for actuator parameter validation."""