Validate actuator arguments so the checks survive -O and None works

Three problems in Actuator.__init__, all reported in #18 and all hit while
building actuator integration tests for BalloonPoppingChallenge.

demand_rate=None is the documented continuous-time mode, but the check read
"demand_rate > 0 or demand_rate is None". Python evaluates the left operand
first, so None > 0 raised TypeError and the mode could not be constructed at all.

The four argument checks were bare asserts, which the interpreter drops under
python -O. I confirmed that a negative time constant, a negative rate limit and
an inverted range were all accepted in silence that way. These read as argument
validation rather than internal invariants, so they now raise ValueError.

actuator_initial_output was assigned without any range check, so an actuator
could start somewhere its own output setter would never have put it, and _reset()
restored that value on every simulation. It now follows the same policy the
setter uses: clamped when clamp is True, warned about when it is False.

The one behaviour change to watch is AssertionError becoming ValueError. The
actuators are fork-only, so the blast radius is this repo's own tests, which are
updated here, and BalloonPoppingChallenge, which does not catch either.

I checked that the clamp cannot move BalloonPoppingChallenge: all three of its
actuators take the default initial output, and 0.0 for gimbal and roll and 1.0
for throttle are inside their ranges, so nothing clamps and no warning fires.

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

Author: thc1006

rocketpy/rocket/actuator/actuator.py

@@ -36,7 +36,9 @@ def __init__(
         clamp : bool, optional
             Whether to clamp the actuator output. Default is True.
         actuator_initial_output : float, optional
-            Initial output of the actuator. Default is 0.0.
+            Initial output of the actuator. Default is 0.0. A value outside
+            actuator_range is treated the way the output setter treats one: it is
+            clamped when clamp is True, and warned about otherwise.
         actuator_time_constant : float, optional
             Time constant of the actuator, implemented as a discrete IIR filter. Default is None.
 
@@ -47,29 +49,42 @@ def __init__(
 
         self.name = name
 
-        assert demand_rate > 0 or demand_rate is None, (
-            "demand_rate must be positive or None."
-        )
+        # 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:
+            raise ValueError("demand_rate must be positive or None.")
         self.demand_rate = demand_rate
 
-        assert actuator_range[0] <= actuator_range[1], (
-            "actuator_range[0] must be <= actuator_range[1]."
-        )
+        if actuator_range[0] > actuator_range[1]:
+            raise ValueError("actuator_range[0] must be <= actuator_range[1].")
         self.actuator_range = actuator_range
 
-        assert actuator_rate_limit is None or actuator_rate_limit >= 0, (
-            "actuator_rate_limit must be non-negative or None."
-        )
+        if actuator_rate_limit is not None and 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
 
-        assert actuator_time_constant is None or actuator_time_constant >= 0, (
-            "actuator_time_constant must be non-negative or None."
-        )
+        if actuator_time_constant is not None and 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()
 
+        # 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.
+        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}."
+            )
+
         self.actuator_initial_output = actuator_initial_output
         self._actuator_output = actuator_initial_output
 

tests/unit/rocket/test_actuators.py

@@ -1,3 +1,6 @@
+import subprocess
+import sys
+
 import pytest
 
 from rocketpy.rocket.actuator.roll import RollActuator
@@ -456,27 +459,104 @@ class TestActuatorValidation:
     """Test suite for actuator parameter validation."""
 
     def test_invalid_demand_rate_negative(self):
-        """Test that negative demand rate raises assertion error."""
-        with pytest.raises(AssertionError):
+        """Test that negative demand rate is rejected."""
+        with pytest.raises(ValueError):
             RollActuator(demand_rate=-1)
 
     def test_invalid_range(self):
-        """Test that invalid range raises assertion error."""
-        with pytest.raises(AssertionError):
+        """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 raises assertion error."""
-        with pytest.raises(AssertionError):
+        """Test that negative time constant is rejected."""
+        with pytest.raises(ValueError):
             ThrottleActuator(throttle_time_constant=-0.1)
 
     def test_invalid_rate_limit_negative(self):
-        """Test that negative rate limit raises assertion error."""
-        with pytest.raises(AssertionError):
+        """Test that negative rate limit is rejected."""
+        with pytest.raises(ValueError):
             ThrustVectorActuator(gimbal_rate_limit=-1.0)
 
+    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, {"demand_rate": -1}),
+            (RollActuator, {"max_roll_torque": -5}),
+            (ThrottleActuator, {"throttle_time_constant": -0.1}),
+            (ThrustVectorActuator, {"gimbal_rate_limit": -1.0}),
+        ],
+    )
+    def test_validation_survives_optimized_mode(self, actuator_class, kwargs):
+        """``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.
+        """
+        source = (
+            "from rocketpy.rocket.actuator import "
+            f"{actuator_class.__name__} as A; A(**{kwargs!r})"
+        )
+        result = subprocess.run(
+            [sys.executable, "-O", "-c", source],
+            capture_output=True,
+            text=True,
+            check=False,
+        )
+
+        assert result.returncode != 0, "invalid arguments were accepted under -O"
+        assert "ValueError" in result.stderr
+
+
+class TestActuatorInitialOutput:
+    """An actuator must not start outside its own range.
+
+    The output setter already refuses to leave the range, but the initial value
+    bypassed it and ``_reset()`` restored that same value, so a reset actuator
+    ended up somewhere the setter would never have put it.
+    """
+
+    def test_an_out_of_range_initial_value_is_clamped(self):
+        actuator = ThrottleActuator(throttle_range=(0.0, 1.0), initial_throttle=2.0)
+
+        assert actuator.actuator_output == 1.0
+        assert actuator.actuator_initial_output == 1.0
+
+    def test_the_clamped_value_survives_a_reset(self):
+        actuator = ThrottleActuator(throttle_range=(0.0, 1.0), initial_throttle=-3.0)
+        actuator.actuator_output = 0.5
+        actuator._reset()
+
+        assert actuator.actuator_output == 0.0
+
+    def test_a_value_inside_the_range_is_untouched(self):
+        actuator = ThrottleActuator(throttle_range=(0.0, 1.0), initial_throttle=0.25)
+
+        assert actuator.actuator_initial_output == 0.25
+
+    def test_without_clamping_it_warns_instead(self):
+        # clamp=False is the documented way to let an actuator report outside its
+        # range, so the initial value follows the setter and only warns.
+        with pytest.warns(UserWarning, match="outside its range"):
+            actuator = ThrottleActuator(
+                throttle_range=(0.0, 1.0), initial_throttle=2.0, clamp=False
+            )
+
+        assert actuator.actuator_initial_output == 2.0
+
 
 class TestActuatorWarnings:
     """Test suite for actuator warning conditions."""