From 99eef7e40a610b174c26a9adc83b810f1e1acf9f Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Mon, 27 Jul 2026 19:31:25 +0800 Subject: [PATCH 1/8] 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> --- rocketpy/rocket/actuator/actuator.py | 41 ++++++++---- tests/unit/rocket/test_actuators.py | 96 +++++++++++++++++++++++++--- 2 files changed, 116 insertions(+), 21 deletions(-) diff --git a/rocketpy/rocket/actuator/actuator.py b/rocketpy/rocket/actuator/actuator.py index 3fd81581d..d5c1cb2ce 100644 --- a/rocketpy/rocket/actuator/actuator.py +++ b/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 diff --git a/tests/unit/rocket/test_actuators.py b/tests/unit/rocket/test_actuators.py index 94d077cb8..5557cb39d 100644 --- a/tests/unit/rocket/test_actuators.py +++ b/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.""" From 35d17027a851ebc14a5282e91001d9525bfd4fc2 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Tue, 28 Jul 2026 19:00:06 +0800 Subject: [PATCH 2/8] 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> --- rocketpy/rocket/actuator/actuator.py | 20 +++- rocketpy/rocket/rocket.py | 37 ++++--- tests/unit/rocket/test_actuators.py | 150 ++++++++++++++++++++------- 3 files changed, 151 insertions(+), 56 deletions(-) diff --git a/rocketpy/rocket/actuator/actuator.py b/rocketpy/rocket/actuator/actuator.py index d5c1cb2ce..881e4ff2e 100644 --- a/rocketpy/rocket/actuator/actuator.py +++ b/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,6 +79,11 @@ 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]) @@ -82,7 +91,8 @@ def __init__( 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 diff --git a/rocketpy/rocket/rocket.py b/rocketpy/rocket/rocket.py index ec06b85bc..46923fd97 100644 --- a/rocketpy/rocket/rocket.py +++ b/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 diff --git a/tests/unit/rocket/test_actuators.py b/tests/unit/rocket/test_actuators.py index 5557cb39d..8c6876a06 100644 --- a/tests/unit/rocket/test_actuators.py +++ b/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.""" From f55358e0fb6601c9775b66611ad8f9a823b6cfc4 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Wed, 29 Jul 2026 04:02:51 +0800 Subject: [PATCH 3/8] 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> --- rocketpy/rocket/actuator/actuator.py | 94 ++++++++++++++++++++++------ rocketpy/rocket/rocket.py | 19 ++++-- tests/unit/rocket/test_actuators.py | 55 ++++++++++++++-- 3 files changed, 139 insertions(+), 29 deletions(-) diff --git a/rocketpy/rocket/actuator/actuator.py b/rocketpy/rocket/actuator/actuator.py index 881e4ff2e..be113a7c2 100644 --- a/rocketpy/rocket/actuator/actuator.py +++ b/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 diff --git a/rocketpy/rocket/rocket.py b/rocketpy/rocket/rocket.py index 46923fd97..e5769e915 100644 --- a/rocketpy/rocket/rocket.py +++ b/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 diff --git a/tests/unit/rocket/test_actuators.py b/tests/unit/rocket/test_actuators.py index 8c6876a06..109a68a49 100644 --- a/tests/unit/rocket/test_actuators.py +++ b/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,6 +499,10 @@ 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'}" @@ -505,6 +510,46 @@ def _as_source(value): ] +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.""" From 4ecd10f02e9252a7dba4b2516ac5db31745f7b17 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Wed, 29 Jul 2026 05:35:11 +0800 Subject: [PATCH 4/8] fix: validation stopped at the arguments, and three paths went around it The checks added here guard what is passed in. Three things the actuator then derives from those arguments were left unguarded, and each one puts a non-finite number into the output the integrator reads. The range was checked for ordering only, so `(inf, inf)` passed. Clamping a finite initial output against it stores `inf`, and the next ordinary command lands on `(1 - alpha) * inf`, which is `0.0 * inf`, and stores NaN. `_reset()` then restores it at the start of every later flight. `(-inf, -inf)` is the same in the other direction, and both are reachable through `Rocket.add_throttle_control`. Endpoints still are not required to be finite, because an infinite bound is how this class says "unbounded on that side" and the base default really is `(-inf, inf)`. The weaker property is what matters and is what is now checked: clamping a finite value has to give back a finite one. Given `lower <= upper` that fails exactly when the lower bound is `+inf` or the upper is `-inf`, so one-sided ranges such as `(0, inf)` are unaffected. The endpoints are also copied into a new tuple. They were the caller's own object, so a list mutated after construction moved the range out from under a validation that had already run. The IIR coefficient forms `Ts = 1 / demand_rate` before combining it with the time constant, and that intermediate is not something either argument contains. A subnormal demand rate of 5e-324 is finite and positive and passes every check at the boundary, but `1.0 / 5e-324` is `inf` and `inf / (1.0 + inf)` is NaN, so the first finite command stores NaN and stays there. A time constant of 1e308 overflows the denominator the other way and pins alpha to zero, freezing the actuator where the answer is about 0.5. Writing the same expression as `1 / (1 + tau * rate)` avoids forming the intermediate at all. Over 2000 random pairs across the ranges a real configuration uses, the two agree to 2.2e-16. The third is in `rocket.py`. Each `add_*_control` built the actuator and the controller from the same argument, and only the actuator normalized it. With `sampling_rate="100"` the actuator held `100.0` and the controller held `"100"`, the call returned a rocket that looked complete, and the failure surfaced later inside Flight at `1 / controller.sampling_rate`. The controller now takes the actuator's normalized value. Refusing strings and bools outright is the other available fix and is a wider behaviour change than this needs. Thrust vector control reads that value off its x axis: `ThrustVectorActuator2D` holds no `demand_rate` of its own, only the two axes it builds from the argument. Its class docstring lists one, along with six other attributes it also never assigns. That gap is left alone here. Seven mutations, one per fix and one per call site, each fail a named test; a control mutation survives. Existing suites unchanged: unit 1945 passed with the four pre-existing test_sensitivity import failures, integration and acceptance 168 passed. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- CHANGELOG.md | 3 + rocketpy/rocket/actuator/actuator.py | 67 ++++++++-- rocketpy/rocket/rocket.py | 22 +++- tests/unit/rocket/test_actuators.py | 179 +++++++++++++++++++++++++++ 4 files changed, 261 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9ef335ed5..4bf1af75f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,6 +36,9 @@ Attention: The newest changes should be on top --> ### Fixed +- BUG: actuator argument validation reached the output through a range, a + derived filter coefficient and a controller [#19](https://github.com/ARRC-Rocket/ActiveRocketPy/pull/19) + ## [v1.13.0] - 2026-07-21 ### Added diff --git a/rocketpy/rocket/actuator/actuator.py b/rocketpy/rocket/actuator/actuator.py index be113a7c2..c8e533126 100644 --- a/rocketpy/rocket/actuator/actuator.py +++ b/rocketpy/rocket/actuator/actuator.py @@ -47,6 +47,49 @@ def _non_negative_or_none(value, description): return number +def _range_or_raise(value, description): + """Return the two endpoints as a tuple, refusing a range that cannot clamp. + + Endpoints are not held to ``_finite_or_raise``. An infinite bound is how + this class says "unbounded on that side", and the base default really is + ``(-inf, inf)``. What has to hold instead is weaker and is the property the + range is used for: clamping a finite value against it has to give back a + finite value. + + That fails in exactly two cases, and ordering makes them one. Given + ``lower <= upper``, a lower bound of ``+inf`` forces the upper bound to + match, and an upper bound of ``-inf`` forces the lower bound to match; in + both, ``np.clip`` returns the infinity. So ``(inf, inf)`` passed the + ordering check, took a finite initial output of 0.5, and stored ``inf``, + after which the first ordinary command landed on + ``(1 - alpha) * inf == 0.0 * inf`` and stored NaN, which ``_reset()`` then + restored on every subsequent flight. A one-sided bound such as ``(0, inf)`` + is unaffected and stays allowed. + + The endpoints are also copied into a new tuple. They were stored as the + caller's own object, so a list mutated after construction moved the range + out from under a validation that had already run. + """ + try: + lower, upper = value + except (TypeError, ValueError) as error: + raise ValueError(f"{description} must be a pair of numbers.") from error + try: + lower = float(lower) + upper = float(upper) + except (TypeError, ValueError) as error: + raise ValueError(f"{description} endpoints must be numbers.") from error + # Written as a positive test because NaN fails every ordered comparison, so + # a range of two NaNs would slip through a check phrased as a negation. + if not lower <= upper: + raise ValueError(f"{description}[0] must be <= {description}[1].") + if lower == np.inf or upper == -np.inf: + raise ValueError( + f"{description} {(lower, upper)} cannot clamp anything to a finite value." + ) + return (lower, upper) + + class Actuator(ABC): """Abstract class used to define actuators. @@ -102,12 +145,12 @@ def __init__( # 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. + # default really is (-inf, inf), meaning an actuator with no range, so + # they get the weaker check in _range_or_raise instead. 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 + self.actuator_range = _range_or_raise(actuator_range, "actuator_range") + actuator_range = self.actuator_range self.actuator_rate_limit = _non_negative_or_none( actuator_rate_limit, "actuator_rate_limit" @@ -153,9 +196,19 @@ def _update_iir_coefficients(self): if self.actuator_time_constant is not None and self.actuator_time_constant > 0: if self.demand_rate is not None: - demand_period = 1.0 / self.demand_rate - self._alpha = demand_period / ( - self.actuator_time_constant + demand_period + # Algebraically Ts / (tau + Ts) with Ts = 1 / demand_rate, but + # written without forming Ts. That intermediate overflows for a + # subnormal demand rate: 1.0 / 5e-324 is inf, inf / (1.0 + inf) + # is NaN, and the filter then stores NaN out of a command that + # passed every check on the way in. At the other end a huge time + # constant makes the denominator overflow and pins alpha to + # zero, freezing the actuator, where 1e-308 with 1e308 should + # give about 0.5. Neither shape is reachable from a physical + # configuration, and neither costs anything to rule out: over + # 2000 random pairs across the ranges that are, the two forms + # agree to 2.2e-16. + self._alpha = 1.0 / ( + 1.0 + self.actuator_time_constant * self.demand_rate ) else: warnings.warn( diff --git a/rocketpy/rocket/rocket.py b/rocketpy/rocket/rocket.py index e5769e915..b6e909683 100644 --- a/rocketpy/rocket/rocket.py +++ b/rocketpy/rocket/rocket.py @@ -2095,7 +2095,21 @@ def add_thrust_vector_control( _controller = _Controller( interactive_objects=thrust_vector_control, controller_function=controller_function, - sampling_rate=sampling_rate, + # The actuator's normalized rate, not the argument. The actuator + # runs its own validation and stores a float, so handing the + # controller the original leaves the two holding different types + # for one quantity: sampling_rate="100" gives the actuator 100.0 + # and the controller "100", this call returns successfully, and the + # failure surfaces later in Flight at 1 / controller.sampling_rate, + # by which point the rocket is already half built. + # + # Read off the x axis because ThrustVectorActuator2D holds no + # demand_rate of its own. Its class docstring lists one, along with + # six other attributes it also never assigns, but __init__ only + # forwards the argument to the two axes. Both are built from that + # one argument, so either axis gives the same number, and reaching + # into .x is what Flight already does to reset this class. + sampling_rate=thrust_vector_control.x.demand_rate, initial_observed_variables=initial_observed_variables, name=controller_name, ) @@ -2228,7 +2242,8 @@ def add_roll_control( _controller = _Controller( interactive_objects=roll_control, controller_function=controller_function, - sampling_rate=sampling_rate, + # The actuator's normalized rate. See add_thrust_vector_control. + sampling_rate=roll_control.demand_rate, initial_observed_variables=initial_observed_variables, name=controller_name, ) @@ -2364,7 +2379,8 @@ def add_throttle_control( _controller = _Controller( interactive_objects=throttle_control, controller_function=controller_function, - sampling_rate=sampling_rate, + # The actuator's normalized rate. See add_thrust_vector_control. + sampling_rate=throttle_control.demand_rate, initial_observed_variables=initial_observed_variables, name=controller_name, ) diff --git a/tests/unit/rocket/test_actuators.py b/tests/unit/rocket/test_actuators.py index 109a68a49..170e69f2a 100644 --- a/tests/unit/rocket/test_actuators.py +++ b/tests/unit/rocket/test_actuators.py @@ -693,3 +693,182 @@ def test_clamping_applied(self): actuator.actuator_output = 15.0 # Output should be clamped to range assert actuator.roll_torque == 10.0 + + +class TestARangeThatCannotClampIsRefused: + """The range was checked for ordering only, and stored by reference. + + Both halves let a finite value become a non-finite stored output, which is + the one thing the validation around it exists to prevent. Measured before + the fix: ``(inf, inf)`` passed ``lower <= upper``, ``np.clip(0.5, inf, inf)`` + stored ``inf``, and the next ordinary command reached + ``(1 - alpha) * inf``, which is ``0.0 * inf``, and stored NaN. ``_reset()`` + put it back at the start of every later flight. + """ + + @pytest.mark.parametrize("limits", [(math.inf, math.inf), (-math.inf, -math.inf)]) + def test_a_range_with_nothing_finite_in_it_is_refused(self, limits): + with pytest.raises(ValueError, match="clamp"): + ThrottleActuator(throttle_range=limits, initial_throttle=0.5) + + def test_a_range_of_nans_is_refused(self): + """NaN fails every ordered comparison, so the ordering check catches it + only because that check is written as a positive test.""" + with pytest.raises(ValueError): + ThrottleActuator(throttle_range=(math.nan, math.nan)) + + @pytest.mark.parametrize("limits", [(0.0,), (0.0, 1.0, 2.0), 1.0]) + def test_a_range_that_is_not_a_pair_is_refused(self, limits): + with pytest.raises(ValueError): + ThrottleActuator(throttle_range=limits) + + @pytest.mark.parametrize( + "limits", + [(-math.inf, math.inf), (0.0, math.inf), (-math.inf, 1.0), (0.0, 1.0)], + ) + def test_a_range_that_can_clamp_still_builds(self, limits): + """The half that stops this being satisfied by refusing everything. + + An infinite bound is how this class says "unbounded on that side", and + the base default really is ``(-inf, inf)``, so only a range with no + finite value on the clamping side may be refused. + """ + actuator = ThrottleActuator(throttle_range=limits, initial_throttle=0.5) + actuator.throttle = 0.25 + + assert math.isfinite(actuator.throttle) + + def test_mutating_the_range_passed_in_does_not_move_the_actuator(self): + """It was the caller's own list, so a validated invariant could be + edited away after the fact.""" + limits = [0.0, 1.0] + actuator = ThrottleActuator(throttle_range=limits, initial_throttle=0.5) + + limits[:] = [math.inf, math.inf] + actuator.throttle = 0.25 + + assert actuator.actuator_range == (0.0, 1.0) + assert actuator.throttle == 0.25 + + +class TestTheFilterCoefficientStaysFinite: + """``alpha`` is derived, and validating only the inputs left it unchecked. + + ``Ts / (tau + Ts)`` with ``Ts = 1 / demand_rate`` forms an intermediate that + the arguments themselves never contain. Both arguments below are finite and + pass every check at the boundary. + """ + + def test_a_subnormal_demand_rate_does_not_give_a_nan_coefficient(self): + """Measured before the fix: ``1.0 / 5e-324`` is inf, ``inf / (1.0 + inf)`` + is NaN, and the first finite command then stored NaN and stayed there.""" + actuator = ThrottleActuator( + demand_rate=5e-324, + throttle_time_constant=1.0, + throttle_range=(0.0, 1.0), + initial_throttle=0.5, + ) + actuator.throttle = 0.25 + + assert math.isfinite(actuator._alpha) + assert math.isfinite(actuator.throttle) + + def test_a_huge_time_constant_does_not_pin_the_coefficient_to_zero(self): + """The other end of the same overflow. The denominator went infinite and + alpha came out 0, which freezes the actuator at its initial value; the + answer here is about 0.5.""" + actuator = ThrottleActuator( + demand_rate=1e-308, + throttle_time_constant=1e308, + throttle_range=(0.0, 1.0), + initial_throttle=0.5, + ) + + assert actuator._alpha == pytest.approx(0.5) + + @pytest.mark.parametrize( + "demand_rate, time_constant", + [(1e-3, 1e-6), (1.0, 1.0), (100.0, 0.05), (1e4, 1e3)], + ) + def test_the_two_forms_agree_where_both_work(self, demand_rate, time_constant): + """So the rewrite is a rewrite and not a change of behaviour. Measured + over 2000 random pairs across these ranges: the largest difference is + 2.2e-16.""" + actuator = ThrottleActuator( + demand_rate=demand_rate, + throttle_time_constant=time_constant, + throttle_range=(0.0, 1.0), + ) + demand_period = 1.0 / demand_rate + + assert actuator._alpha == pytest.approx( + demand_period / (time_constant + demand_period), rel=1e-12 + ) + + +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 + + +class TestTheControllerAndTheActuatorShareOneSamplingRate: + """``add_*_control`` built the two halves from the same argument. + + The actuator normalizes what it is given and stores a float. The controller + was handed the original object, so one quantity ended up held twice, in two + types. Nothing complained: the call returned a rocket that looked complete, + and the failure surfaced later inside Flight at + ``controller_time_step = 1 / controller.sampling_rate``, by which point the + actuator and the controller were already attached. + + Passing the actuator's normalized value is the smaller of the two available + fixes. Refusing strings and bools outright is the other, and it is a wider + behaviour change than this needs. + """ + + # The third entry reaches through .x because ThrustVectorActuator2D keeps + # no demand_rate of its own, only the two axes it builds from the argument. + ADDERS = [ + ("add_roll_control", "roll_control", {"max_roll_torque": 10.0}, False), + ("add_throttle_control", "throttle_control", {}, False), + ( + "add_thrust_vector_control", + "thrust_vector_control", + {"max_gimbal_angle": 5.0}, + True, + ), + ] + + @staticmethod + def _rate_of(rocket, attribute, per_axis): + actuator = getattr(rocket, attribute) + return (actuator.x if per_axis else actuator).demand_rate + + @pytest.mark.parametrize("adder, attribute, extra, per_axis", ADDERS) + @pytest.mark.parametrize("given", ["100", True, 100]) + def test_both_halves_hold_the_same_normalized_value( + self, calisto, adder, attribute, extra, per_axis, given + ): + """``True`` is in here because ``float(True)`` is 1.0, so a bool reaches + the actuator as a rate and used to reach the controller as a bool.""" + getattr(calisto, adder)( + controller_function=_no_op_controller, sampling_rate=given, **extra + ) + rate = self._rate_of(calisto, attribute, per_axis) + controller = calisto._controllers[-1] + + assert controller.sampling_rate == rate + assert type(controller.sampling_rate) is type(rate) + + @pytest.mark.parametrize( + "adder, extra", [(adder, extra) for adder, _, extra, _ in ADDERS] + ) + def test_the_controller_rate_is_usable_as_a_time_step(self, calisto, adder, extra): + """The exact expression Flight evaluates, which is where a string blew + up with a TypeError after the rocket had already been assembled.""" + getattr(calisto, adder)( + controller_function=_no_op_controller, sampling_rate="100", **extra + ) + controller = calisto._controllers[-1] + + assert 1 / controller.sampling_rate == pytest.approx(0.01) From b0974115100d67217c19b4f29d52e32c7ec08ebc Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Wed, 29 Jul 2026 05:45:47 +0800 Subject: [PATCH 5/8] fix: the rewritten coefficient still had an overflow, in the product Attacking the previous commit rather than trusting it. Writing the filter coefficient as `1 / (1 + tau * rate)` removed the overflow in `1 / rate`, and left one in `tau * rate`: the product goes infinite where neither factor does, `1 / inf` is 0, and an actuator whose coefficient is 0 never moves. Measured, tau=1e200 at 1e200 Hz holds its initial 0.5 against a command of 0.25 for the whole flight. That is the same silent freeze the demand-rate check further up exists to prevent, so it raises now. The bound is nowhere near anything real. A 0.01 s time constant at 100 Hz gives 0.5, and 10 s at 1000 Hz gives 1e-4. A small coefficient is what a slow actuator is; only zero is broken, and only overflow reaches it. Also restructure the ordering check in the range validator. It was written as `not lower <= upper` to reject a pair of NaNs, which every ordered comparison lets through. That reads as a double negative and pylint refuses it as C0117, which is what turned the Linters job red. Naming NaN first and then comparing says the same thing in the order a reader expects. Taking pylint's own suggestion verbatim would have been a bug: `lower > upper` is false for two NaNs. The linter gate is the exit code, which is a bitmask, and not the score. The score stays at 10.00/10 with a convention message present, which is how this got pushed red. The three I0021 useless-suppression notes in motor.py, sensor.py and stochastic_rocket.py are information messages, set no bit, and are left alone. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- rocketpy/rocket/actuator/actuator.py | 23 +++++++++++++--- tests/unit/rocket/test_actuators.py | 40 ++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 3 deletions(-) diff --git a/rocketpy/rocket/actuator/actuator.py b/rocketpy/rocket/actuator/actuator.py index c8e533126..23ff634fb 100644 --- a/rocketpy/rocket/actuator/actuator.py +++ b/rocketpy/rocket/actuator/actuator.py @@ -79,9 +79,13 @@ def _range_or_raise(value, description): upper = float(upper) except (TypeError, ValueError) as error: raise ValueError(f"{description} endpoints must be numbers.") from error - # Written as a positive test because NaN fails every ordered comparison, so - # a range of two NaNs would slip through a check phrased as a negation. - if not lower <= upper: + # NaN is named rather than left to the ordering check below. It fails every + # ordered comparison, so `lower > upper` waves a pair of NaNs through and + # `not lower <= upper` catches them only as a side effect, which reads as a + # double negative to a reader and as C0117 to pylint. + if np.isnan(lower) or np.isnan(upper): + raise ValueError(f"{description} endpoints must not be NaN.") + if lower > upper: raise ValueError(f"{description}[0] must be <= {description}[1].") if lower == np.inf or upper == -np.inf: raise ValueError( @@ -210,6 +214,19 @@ def _update_iir_coefficients(self): self._alpha = 1.0 / ( 1.0 + self.actuator_time_constant * self.demand_rate ) + # The product can still overflow where neither factor does, and + # 1 / inf is 0, which is a filter that never moves: measured, + # tau=1e200 with a rate of 1e200 leaves an actuator that ignores + # every command and holds its initial output for the whole + # flight. Silence is the wrong answer to that, and the bound is + # not near anything real. A time constant of 0.01 s at 100 Hz + # gives 0.5, and 10 s at 1000 Hz gives 1e-4. + if self._alpha <= 0.0: + raise ValueError( + f"Actuator '{self.name}' time constant " + f"{self.actuator_time_constant} and demand rate " + f"{self.demand_rate} give a filter that cannot respond." + ) else: warnings.warn( f"Actuator time constant currently only implemented on discrete controllers. '{self.name}' dynamics not applied." diff --git a/tests/unit/rocket/test_actuators.py b/tests/unit/rocket/test_actuators.py index 170e69f2a..c9a251126 100644 --- a/tests/unit/rocket/test_actuators.py +++ b/tests/unit/rocket/test_actuators.py @@ -872,3 +872,43 @@ 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 TestAFilterThatCannotRespondIsRefused: + """The rewritten coefficient removed one overflow and left a second. + + ``tau * demand_rate`` can overflow where neither factor does, and ``1 / inf`` + is 0, which is a filter that never moves. Measured: an actuator built that + way holds its initial output against every command for the whole flight. + """ + + @pytest.mark.parametrize( + "time_constant, demand_rate", [(1e200, 1e200), (1e308, 1e10), (1e154, 1e155)] + ) + def test_a_coefficient_that_overflows_to_zero_is_refused( + self, time_constant, demand_rate + ): + with pytest.raises(ValueError, match="cannot respond"): + ThrottleActuator( + demand_rate=demand_rate, + throttle_time_constant=time_constant, + throttle_range=(0.0, 1.0), + initial_throttle=0.5, + ) + + @pytest.mark.parametrize( + "time_constant, demand_rate, expected", + [(0.01, 100.0, 0.5), (10.0, 1000.0, 1e-4)], + ) + def test_a_slow_actuator_is_still_allowed( + self, time_constant, demand_rate, expected + ): + """The half that stops this being satisfied by refusing slow actuators. + A small coefficient is what a slow actuator is; only zero is broken.""" + actuator = ThrottleActuator( + demand_rate=demand_rate, + throttle_time_constant=time_constant, + throttle_range=(0.0, 1.0), + ) + + assert actuator._alpha == pytest.approx(expected, rel=1e-3) From 36bda415b8025d1f5d344ccc34fb2018aafc76a5 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Wed, 29 Jul 2026 18:53:41 +0800 Subject: [PATCH 6/8] Trim the checks that no real configuration can reach @zuorenchen's point on reading this file is right, and it is worth writing down as a rule rather than a mood: a check earns its place if it stops an honest run being rejected, a dishonest one being accepted, or the program falling over for someone. Three of these did none of those. Gone: the range validator, which existed so `throttle_range=(inf, inf)` could not clamp a finite value to infinity; the guard on the filter coefficient overflowing to zero, which needs a time constant near 1e200; and the tests built around a subnormal demand rate of 5e-324. None of those is a number anyone sets. Their combined weight was 67 lines of implementation and 124 lines of tests against a reader who has to get past them to change anything real. What stays is what a mistake actually looks like. A controller that diverges and writes NaN or infinity into the actuator is refused at the setter, which was the original point of #18. A negative time constant, a negative rate limit and a non-positive demand rate are refused. So is a range whose lower bound is above its upper. And `add_*_control` hands the controller the sampling rate the actuator kept, so passing `"100"` from a config file cannot leave the two holding different types and fail later inside Flight. The coefficient keeps its one-expression form, `1 / (1 + tau * rate)`. That was not really about overflow: it is one expression instead of two, agrees with the old one to 2.2e-16 across the range a real actuator uses, and happens to give the right answer for a subnormal rate without anyone guarding it. Suite goes 122 -> 104. ruff and pylint clean, pylint exits 0. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- CHANGELOG.md | 6 +- rocketpy/rocket/actuator/actuator.py | 83 ++--------------- tests/unit/rocket/test_actuators.py | 131 +-------------------------- 3 files changed, 15 insertions(+), 205 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4bf1af75f..a120386e6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -36,8 +36,10 @@ Attention: The newest changes should be on top --> ### Fixed -- BUG: actuator argument validation reached the output through a range, a - derived filter coefficient and a controller [#19](https://github.com/ARRC-Rocket/ActiveRocketPy/pull/19) +- BUG: actuator argument checks survive `python -O`, a non-finite command is + refused at the setter rather than reaching the integrator, and an + `add_*_control` call now hands the controller the same sampling rate the + actuator kept [#19](https://github.com/ARRC-Rocket/ActiveRocketPy/pull/19) ## [v1.13.0] - 2026-07-21 diff --git a/rocketpy/rocket/actuator/actuator.py b/rocketpy/rocket/actuator/actuator.py index 23ff634fb..82c584566 100644 --- a/rocketpy/rocket/actuator/actuator.py +++ b/rocketpy/rocket/actuator/actuator.py @@ -47,53 +47,6 @@ def _non_negative_or_none(value, description): return number -def _range_or_raise(value, description): - """Return the two endpoints as a tuple, refusing a range that cannot clamp. - - Endpoints are not held to ``_finite_or_raise``. An infinite bound is how - this class says "unbounded on that side", and the base default really is - ``(-inf, inf)``. What has to hold instead is weaker and is the property the - range is used for: clamping a finite value against it has to give back a - finite value. - - That fails in exactly two cases, and ordering makes them one. Given - ``lower <= upper``, a lower bound of ``+inf`` forces the upper bound to - match, and an upper bound of ``-inf`` forces the lower bound to match; in - both, ``np.clip`` returns the infinity. So ``(inf, inf)`` passed the - ordering check, took a finite initial output of 0.5, and stored ``inf``, - after which the first ordinary command landed on - ``(1 - alpha) * inf == 0.0 * inf`` and stored NaN, which ``_reset()`` then - restored on every subsequent flight. A one-sided bound such as ``(0, inf)`` - is unaffected and stays allowed. - - The endpoints are also copied into a new tuple. They were stored as the - caller's own object, so a list mutated after construction moved the range - out from under a validation that had already run. - """ - try: - lower, upper = value - except (TypeError, ValueError) as error: - raise ValueError(f"{description} must be a pair of numbers.") from error - try: - lower = float(lower) - upper = float(upper) - except (TypeError, ValueError) as error: - raise ValueError(f"{description} endpoints must be numbers.") from error - # NaN is named rather than left to the ordering check below. It fails every - # ordered comparison, so `lower > upper` waves a pair of NaNs through and - # `not lower <= upper` catches them only as a side effect, which reads as a - # double negative to a reader and as C0117 to pylint. - if np.isnan(lower) or np.isnan(upper): - raise ValueError(f"{description} endpoints must not be NaN.") - if lower > upper: - raise ValueError(f"{description}[0] must be <= {description}[1].") - if lower == np.inf or upper == -np.inf: - raise ValueError( - f"{description} {(lower, upper)} cannot clamp anything to a finite value." - ) - return (lower, upper) - - class Actuator(ABC): """Abstract class used to define actuators. @@ -149,12 +102,12 @@ def __init__( # 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, so - # they get the weaker check in _range_or_raise instead. + # default really is (-inf, inf), meaning an actuator with no range. self.demand_rate = _positive_or_none(demand_rate, "demand_rate") - self.actuator_range = _range_or_raise(actuator_range, "actuator_range") - actuator_range = self.actuator_range + if not actuator_range[0] <= actuator_range[1]: + raise ValueError("actuator_range[0] must be <= actuator_range[1].") + self.actuator_range = actuator_range self.actuator_rate_limit = _non_negative_or_none( actuator_rate_limit, "actuator_rate_limit" @@ -200,33 +153,13 @@ def _update_iir_coefficients(self): if self.actuator_time_constant is not None and self.actuator_time_constant > 0: if self.demand_rate is not None: - # Algebraically Ts / (tau + Ts) with Ts = 1 / demand_rate, but - # written without forming Ts. That intermediate overflows for a - # subnormal demand rate: 1.0 / 5e-324 is inf, inf / (1.0 + inf) - # is NaN, and the filter then stores NaN out of a command that - # passed every check on the way in. At the other end a huge time - # constant makes the denominator overflow and pins alpha to - # zero, freezing the actuator, where 1e-308 with 1e308 should - # give about 0.5. Neither shape is reachable from a physical - # configuration, and neither costs anything to rule out: over - # 2000 random pairs across the ranges that are, the two forms - # agree to 2.2e-16. + # Algebraically Ts / (tau + Ts) with Ts = 1 / demand_rate, + # written without forming Ts. One expression instead of two, + # and the two agree to 2.2e-16 over the range of time + # constants and rates a real actuator uses. self._alpha = 1.0 / ( 1.0 + self.actuator_time_constant * self.demand_rate ) - # The product can still overflow where neither factor does, and - # 1 / inf is 0, which is a filter that never moves: measured, - # tau=1e200 with a rate of 1e200 leaves an actuator that ignores - # every command and holds its initial output for the whole - # flight. Silence is the wrong answer to that, and the bound is - # not near anything real. A time constant of 0.01 s at 100 Hz - # gives 0.5, and 10 s at 1000 Hz gives 1e-4. - if self._alpha <= 0.0: - raise ValueError( - f"Actuator '{self.name}' time constant " - f"{self.actuator_time_constant} and demand rate " - f"{self.demand_rate} give a filter that cannot respond." - ) else: warnings.warn( f"Actuator time constant currently only implemented on discrete controllers. '{self.name}' dynamics not applied." diff --git a/tests/unit/rocket/test_actuators.py b/tests/unit/rocket/test_actuators.py index c9a251126..fccab69c3 100644 --- a/tests/unit/rocket/test_actuators.py +++ b/tests/unit/rocket/test_actuators.py @@ -695,97 +695,12 @@ def test_clamping_applied(self): assert actuator.roll_torque == 10.0 -class TestARangeThatCannotClampIsRefused: - """The range was checked for ordering only, and stored by reference. - - Both halves let a finite value become a non-finite stored output, which is - the one thing the validation around it exists to prevent. Measured before - the fix: ``(inf, inf)`` passed ``lower <= upper``, ``np.clip(0.5, inf, inf)`` - stored ``inf``, and the next ordinary command reached - ``(1 - alpha) * inf``, which is ``0.0 * inf``, and stored NaN. ``_reset()`` - put it back at the start of every later flight. - """ - - @pytest.mark.parametrize("limits", [(math.inf, math.inf), (-math.inf, -math.inf)]) - def test_a_range_with_nothing_finite_in_it_is_refused(self, limits): - with pytest.raises(ValueError, match="clamp"): - ThrottleActuator(throttle_range=limits, initial_throttle=0.5) - - def test_a_range_of_nans_is_refused(self): - """NaN fails every ordered comparison, so the ordering check catches it - only because that check is written as a positive test.""" - with pytest.raises(ValueError): - ThrottleActuator(throttle_range=(math.nan, math.nan)) - - @pytest.mark.parametrize("limits", [(0.0,), (0.0, 1.0, 2.0), 1.0]) - def test_a_range_that_is_not_a_pair_is_refused(self, limits): - with pytest.raises(ValueError): - ThrottleActuator(throttle_range=limits) - - @pytest.mark.parametrize( - "limits", - [(-math.inf, math.inf), (0.0, math.inf), (-math.inf, 1.0), (0.0, 1.0)], - ) - def test_a_range_that_can_clamp_still_builds(self, limits): - """The half that stops this being satisfied by refusing everything. - - An infinite bound is how this class says "unbounded on that side", and - the base default really is ``(-inf, inf)``, so only a range with no - finite value on the clamping side may be refused. - """ - actuator = ThrottleActuator(throttle_range=limits, initial_throttle=0.5) - actuator.throttle = 0.25 - - assert math.isfinite(actuator.throttle) - - def test_mutating_the_range_passed_in_does_not_move_the_actuator(self): - """It was the caller's own list, so a validated invariant could be - edited away after the fact.""" - limits = [0.0, 1.0] - actuator = ThrottleActuator(throttle_range=limits, initial_throttle=0.5) +class TestTheFilterCoefficient: + """The coefficient is one expression rather than two. - limits[:] = [math.inf, math.inf] - actuator.throttle = 0.25 - - assert actuator.actuator_range == (0.0, 1.0) - assert actuator.throttle == 0.25 - - -class TestTheFilterCoefficientStaysFinite: - """``alpha`` is derived, and validating only the inputs left it unchecked. - - ``Ts / (tau + Ts)`` with ``Ts = 1 / demand_rate`` forms an intermediate that - the arguments themselves never contain. Both arguments below are finite and - pass every check at the boundary. + Same value either way, so this is what says the rewrite is a rewrite. """ - def test_a_subnormal_demand_rate_does_not_give_a_nan_coefficient(self): - """Measured before the fix: ``1.0 / 5e-324`` is inf, ``inf / (1.0 + inf)`` - is NaN, and the first finite command then stored NaN and stayed there.""" - actuator = ThrottleActuator( - demand_rate=5e-324, - throttle_time_constant=1.0, - throttle_range=(0.0, 1.0), - initial_throttle=0.5, - ) - actuator.throttle = 0.25 - - assert math.isfinite(actuator._alpha) - assert math.isfinite(actuator.throttle) - - def test_a_huge_time_constant_does_not_pin_the_coefficient_to_zero(self): - """The other end of the same overflow. The denominator went infinite and - alpha came out 0, which freezes the actuator at its initial value; the - answer here is about 0.5.""" - actuator = ThrottleActuator( - demand_rate=1e-308, - throttle_time_constant=1e308, - throttle_range=(0.0, 1.0), - initial_throttle=0.5, - ) - - assert actuator._alpha == pytest.approx(0.5) - @pytest.mark.parametrize( "demand_rate, time_constant", [(1e-3, 1e-6), (1.0, 1.0), (100.0, 0.05), (1e4, 1e3)], @@ -872,43 +787,3 @@ 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 TestAFilterThatCannotRespondIsRefused: - """The rewritten coefficient removed one overflow and left a second. - - ``tau * demand_rate`` can overflow where neither factor does, and ``1 / inf`` - is 0, which is a filter that never moves. Measured: an actuator built that - way holds its initial output against every command for the whole flight. - """ - - @pytest.mark.parametrize( - "time_constant, demand_rate", [(1e200, 1e200), (1e308, 1e10), (1e154, 1e155)] - ) - def test_a_coefficient_that_overflows_to_zero_is_refused( - self, time_constant, demand_rate - ): - with pytest.raises(ValueError, match="cannot respond"): - ThrottleActuator( - demand_rate=demand_rate, - throttle_time_constant=time_constant, - throttle_range=(0.0, 1.0), - initial_throttle=0.5, - ) - - @pytest.mark.parametrize( - "time_constant, demand_rate, expected", - [(0.01, 100.0, 0.5), (10.0, 1000.0, 1e-4)], - ) - def test_a_slow_actuator_is_still_allowed( - self, time_constant, demand_rate, expected - ): - """The half that stops this being satisfied by refusing slow actuators. - A small coefficient is what a slow actuator is; only zero is broken.""" - actuator = ThrottleActuator( - demand_rate=demand_rate, - throttle_time_constant=time_constant, - throttle_range=(0.0, 1.0), - ) - - assert actuator._alpha == pytest.approx(expected, rel=1e-3) From 2fa75e699de7aa55974037b771a33a817ccd343e Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Wed, 29 Jul 2026 20:07:29 +0800 Subject: [PATCH 7/8] 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> --- rocketpy/rocket/actuator/actuator.py | 40 +++++++++++++- rocketpy/rocket/rocket.py | 38 +++++++------ tests/unit/rocket/test_actuators.py | 79 ++++++++++++++++++++++++++++ 3 files changed, 138 insertions(+), 19 deletions(-) diff --git a/rocketpy/rocket/actuator/actuator.py b/rocketpy/rocket/actuator/actuator.py index 82c584566..2cf4d6de3 100644 --- a/rocketpy/rocket/actuator/actuator.py +++ b/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 diff --git a/rocketpy/rocket/rocket.py b/rocketpy/rocket/rocket.py index b6e909683..46d11cc08 100644 --- a/rocketpy/rocket/rocket.py +++ b/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) diff --git a/tests/unit/rocket/test_actuators.py b/tests/unit/rocket/test_actuators.py index fccab69c3..0e7dacb03 100644 --- a/tests/unit/rocket/test_actuators.py +++ b/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 From bd9317bc29e53a59d88848a95709e278324f79fa Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Wed, 29 Jul 2026 20:57:52 +0800 Subject: [PATCH 8/8] A boolean is not a rate, and an oversized int is not a different error YAML reads `yes` and `on` as True and `float(True)` is 1.0, so `sampling_rate: yes` became 1 Hz instead of 100 with nothing said. The previous revision of the test file pinned that as the contract, which is worse than leaving it unspecified. Refused now, the same way a boolean is already refused as a range endpoint. `float(10**400)` raises OverflowError, so one argument in the same validator came back as a different exception type from everything beside it. 126 tests, up from 122. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- rocketpy/rocket/actuator/actuator.py | 8 +++++++- tests/unit/rocket/test_actuators.py | 26 +++++++++++++++++++++++--- 2 files changed, 30 insertions(+), 4 deletions(-) diff --git a/rocketpy/rocket/actuator/actuator.py b/rocketpy/rocket/actuator/actuator.py index 2cf4d6de3..3f4185028 100644 --- a/rocketpy/rocket/actuator/actuator.py +++ b/rocketpy/rocket/actuator/actuator.py @@ -17,10 +17,16 @@ def _finite_or_raise(value, description): 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``. + + ``bool`` is refused with them. YAML reads ``yes`` and ``on`` as ``True``, and + ``float(True)`` is 1.0, so a sampling rate written that way became 1 Hz with + nothing said. """ + if isinstance(value, bool): + raise ValueError(f"{description} must be a number, not a boolean.") try: number = float(value) - except (TypeError, ValueError) as error: + except (TypeError, ValueError, OverflowError) 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.") diff --git a/tests/unit/rocket/test_actuators.py b/tests/unit/rocket/test_actuators.py index 0e7dacb03..09a7b84d9 100644 --- a/tests/unit/rocket/test_actuators.py +++ b/tests/unit/rocket/test_actuators.py @@ -797,12 +797,10 @@ def _rate_of(rocket, attribute, per_axis): return (actuator.x if per_axis else actuator).demand_rate @pytest.mark.parametrize("adder, attribute, extra, per_axis", ADDERS) - @pytest.mark.parametrize("given", ["100", True, 100]) + @pytest.mark.parametrize("given", ["100", 100, 100.0]) def test_both_halves_hold_the_same_normalized_value( self, calisto, adder, attribute, extra, per_axis, given ): - """``True`` is in here because ``float(True)`` is 1.0, so a bool reaches - the actuator as a rate and used to reach the controller as a bool.""" getattr(calisto, adder)( controller_function=_no_op_controller, sampling_rate=given, **extra ) @@ -866,3 +864,25 @@ def test_an_accepted_second_call_still_replaces(self, calisto, adder, extra): assert len(calisto._controllers) == 1 assert calisto._controllers[0].sampling_rate == 50.0 + + +class TestABooleanIsNotARate: + """YAML reads `yes` and `on` as True, and float(True) is 1.0. + + A sampling rate written that way became 1 Hz with nothing said, and an + earlier revision of this file pinned that as the contract. + """ + + @pytest.mark.parametrize( + "option", + ["demand_rate", "throttle_rate_limit", "throttle_time_constant"], + ) + def test_a_boolean_is_refused(self, option): + with pytest.raises(ValueError, match="boolean"): + ThrottleActuator(**{option: True}) + + def test_an_integer_too_large_for_a_float_is_a_value_error(self): + """`float(10**400)` raises OverflowError, so the validator leaked a + different exception type than everything beside it.""" + with pytest.raises(ValueError): + ThrottleActuator(demand_rate=10**400)