Skip to content

Commit 36bda41

Browse files
committed
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>
1 parent b097411 commit 36bda41

3 files changed

Lines changed: 15 additions & 205 deletions

File tree

CHANGELOG.md

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -36,8 +36,10 @@ Attention: The newest changes should be on top -->
3636

3737
### Fixed
3838

39-
- BUG: actuator argument validation reached the output through a range, a
40-
derived filter coefficient and a controller [#19](https://github.com/ARRC-Rocket/ActiveRocketPy/pull/19)
39+
- BUG: actuator argument checks survive `python -O`, a non-finite command is
40+
refused at the setter rather than reaching the integrator, and an
41+
`add_*_control` call now hands the controller the same sampling rate the
42+
actuator kept [#19](https://github.com/ARRC-Rocket/ActiveRocketPy/pull/19)
4143

4244
## [v1.13.0] - 2026-07-21
4345

rocketpy/rocket/actuator/actuator.py

Lines changed: 8 additions & 75 deletions
Original file line numberDiff line numberDiff line change
@@ -47,53 +47,6 @@ def _non_negative_or_none(value, description):
4747
return number
4848

4949

50-
def _range_or_raise(value, description):
51-
"""Return the two endpoints as a tuple, refusing a range that cannot clamp.
52-
53-
Endpoints are not held to ``_finite_or_raise``. An infinite bound is how
54-
this class says "unbounded on that side", and the base default really is
55-
``(-inf, inf)``. What has to hold instead is weaker and is the property the
56-
range is used for: clamping a finite value against it has to give back a
57-
finite value.
58-
59-
That fails in exactly two cases, and ordering makes them one. Given
60-
``lower <= upper``, a lower bound of ``+inf`` forces the upper bound to
61-
match, and an upper bound of ``-inf`` forces the lower bound to match; in
62-
both, ``np.clip`` returns the infinity. So ``(inf, inf)`` passed the
63-
ordering check, took a finite initial output of 0.5, and stored ``inf``,
64-
after which the first ordinary command landed on
65-
``(1 - alpha) * inf == 0.0 * inf`` and stored NaN, which ``_reset()`` then
66-
restored on every subsequent flight. A one-sided bound such as ``(0, inf)``
67-
is unaffected and stays allowed.
68-
69-
The endpoints are also copied into a new tuple. They were stored as the
70-
caller's own object, so a list mutated after construction moved the range
71-
out from under a validation that had already run.
72-
"""
73-
try:
74-
lower, upper = value
75-
except (TypeError, ValueError) as error:
76-
raise ValueError(f"{description} must be a pair of numbers.") from error
77-
try:
78-
lower = float(lower)
79-
upper = float(upper)
80-
except (TypeError, ValueError) as error:
81-
raise ValueError(f"{description} endpoints must be numbers.") from error
82-
# NaN is named rather than left to the ordering check below. It fails every
83-
# ordered comparison, so `lower > upper` waves a pair of NaNs through and
84-
# `not lower <= upper` catches them only as a side effect, which reads as a
85-
# double negative to a reader and as C0117 to pylint.
86-
if np.isnan(lower) or np.isnan(upper):
87-
raise ValueError(f"{description} endpoints must not be NaN.")
88-
if lower > upper:
89-
raise ValueError(f"{description}[0] must be <= {description}[1].")
90-
if lower == np.inf or upper == -np.inf:
91-
raise ValueError(
92-
f"{description} {(lower, upper)} cannot clamp anything to a finite value."
93-
)
94-
return (lower, upper)
95-
96-
9750
class Actuator(ABC):
9851
"""Abstract class used to define actuators.
9952
@@ -149,12 +102,12 @@ def __init__(
149102
# zero and freezes the output.
150103
#
151104
# The range endpoints are deliberately not put through this. The base
152-
# default really is (-inf, inf), meaning an actuator with no range, so
153-
# they get the weaker check in _range_or_raise instead.
105+
# default really is (-inf, inf), meaning an actuator with no range.
154106
self.demand_rate = _positive_or_none(demand_rate, "demand_rate")
155107

156-
self.actuator_range = _range_or_raise(actuator_range, "actuator_range")
157-
actuator_range = self.actuator_range
108+
if not actuator_range[0] <= actuator_range[1]:
109+
raise ValueError("actuator_range[0] must be <= actuator_range[1].")
110+
self.actuator_range = actuator_range
158111

159112
self.actuator_rate_limit = _non_negative_or_none(
160113
actuator_rate_limit, "actuator_rate_limit"
@@ -200,33 +153,13 @@ def _update_iir_coefficients(self):
200153

201154
if self.actuator_time_constant is not None and self.actuator_time_constant > 0:
202155
if self.demand_rate is not None:
203-
# Algebraically Ts / (tau + Ts) with Ts = 1 / demand_rate, but
204-
# written without forming Ts. That intermediate overflows for a
205-
# subnormal demand rate: 1.0 / 5e-324 is inf, inf / (1.0 + inf)
206-
# is NaN, and the filter then stores NaN out of a command that
207-
# passed every check on the way in. At the other end a huge time
208-
# constant makes the denominator overflow and pins alpha to
209-
# zero, freezing the actuator, where 1e-308 with 1e308 should
210-
# give about 0.5. Neither shape is reachable from a physical
211-
# configuration, and neither costs anything to rule out: over
212-
# 2000 random pairs across the ranges that are, the two forms
213-
# agree to 2.2e-16.
156+
# Algebraically Ts / (tau + Ts) with Ts = 1 / demand_rate,
157+
# written without forming Ts. One expression instead of two,
158+
# and the two agree to 2.2e-16 over the range of time
159+
# constants and rates a real actuator uses.
214160
self._alpha = 1.0 / (
215161
1.0 + self.actuator_time_constant * self.demand_rate
216162
)
217-
# The product can still overflow where neither factor does, and
218-
# 1 / inf is 0, which is a filter that never moves: measured,
219-
# tau=1e200 with a rate of 1e200 leaves an actuator that ignores
220-
# every command and holds its initial output for the whole
221-
# flight. Silence is the wrong answer to that, and the bound is
222-
# not near anything real. A time constant of 0.01 s at 100 Hz
223-
# gives 0.5, and 10 s at 1000 Hz gives 1e-4.
224-
if self._alpha <= 0.0:
225-
raise ValueError(
226-
f"Actuator '{self.name}' time constant "
227-
f"{self.actuator_time_constant} and demand rate "
228-
f"{self.demand_rate} give a filter that cannot respond."
229-
)
230163
else:
231164
warnings.warn(
232165
f"Actuator time constant currently only implemented on discrete controllers. '{self.name}' dynamics not applied."

tests/unit/rocket/test_actuators.py

Lines changed: 3 additions & 128 deletions
Original file line numberDiff line numberDiff line change
@@ -695,97 +695,12 @@ def test_clamping_applied(self):
695695
assert actuator.roll_torque == 10.0
696696

697697

698-
class TestARangeThatCannotClampIsRefused:
699-
"""The range was checked for ordering only, and stored by reference.
700-
701-
Both halves let a finite value become a non-finite stored output, which is
702-
the one thing the validation around it exists to prevent. Measured before
703-
the fix: ``(inf, inf)`` passed ``lower <= upper``, ``np.clip(0.5, inf, inf)``
704-
stored ``inf``, and the next ordinary command reached
705-
``(1 - alpha) * inf``, which is ``0.0 * inf``, and stored NaN. ``_reset()``
706-
put it back at the start of every later flight.
707-
"""
708-
709-
@pytest.mark.parametrize("limits", [(math.inf, math.inf), (-math.inf, -math.inf)])
710-
def test_a_range_with_nothing_finite_in_it_is_refused(self, limits):
711-
with pytest.raises(ValueError, match="clamp"):
712-
ThrottleActuator(throttle_range=limits, initial_throttle=0.5)
713-
714-
def test_a_range_of_nans_is_refused(self):
715-
"""NaN fails every ordered comparison, so the ordering check catches it
716-
only because that check is written as a positive test."""
717-
with pytest.raises(ValueError):
718-
ThrottleActuator(throttle_range=(math.nan, math.nan))
719-
720-
@pytest.mark.parametrize("limits", [(0.0,), (0.0, 1.0, 2.0), 1.0])
721-
def test_a_range_that_is_not_a_pair_is_refused(self, limits):
722-
with pytest.raises(ValueError):
723-
ThrottleActuator(throttle_range=limits)
724-
725-
@pytest.mark.parametrize(
726-
"limits",
727-
[(-math.inf, math.inf), (0.0, math.inf), (-math.inf, 1.0), (0.0, 1.0)],
728-
)
729-
def test_a_range_that_can_clamp_still_builds(self, limits):
730-
"""The half that stops this being satisfied by refusing everything.
731-
732-
An infinite bound is how this class says "unbounded on that side", and
733-
the base default really is ``(-inf, inf)``, so only a range with no
734-
finite value on the clamping side may be refused.
735-
"""
736-
actuator = ThrottleActuator(throttle_range=limits, initial_throttle=0.5)
737-
actuator.throttle = 0.25
738-
739-
assert math.isfinite(actuator.throttle)
740-
741-
def test_mutating_the_range_passed_in_does_not_move_the_actuator(self):
742-
"""It was the caller's own list, so a validated invariant could be
743-
edited away after the fact."""
744-
limits = [0.0, 1.0]
745-
actuator = ThrottleActuator(throttle_range=limits, initial_throttle=0.5)
698+
class TestTheFilterCoefficient:
699+
"""The coefficient is one expression rather than two.
746700
747-
limits[:] = [math.inf, math.inf]
748-
actuator.throttle = 0.25
749-
750-
assert actuator.actuator_range == (0.0, 1.0)
751-
assert actuator.throttle == 0.25
752-
753-
754-
class TestTheFilterCoefficientStaysFinite:
755-
"""``alpha`` is derived, and validating only the inputs left it unchecked.
756-
757-
``Ts / (tau + Ts)`` with ``Ts = 1 / demand_rate`` forms an intermediate that
758-
the arguments themselves never contain. Both arguments below are finite and
759-
pass every check at the boundary.
701+
Same value either way, so this is what says the rewrite is a rewrite.
760702
"""
761703

762-
def test_a_subnormal_demand_rate_does_not_give_a_nan_coefficient(self):
763-
"""Measured before the fix: ``1.0 / 5e-324`` is inf, ``inf / (1.0 + inf)``
764-
is NaN, and the first finite command then stored NaN and stayed there."""
765-
actuator = ThrottleActuator(
766-
demand_rate=5e-324,
767-
throttle_time_constant=1.0,
768-
throttle_range=(0.0, 1.0),
769-
initial_throttle=0.5,
770-
)
771-
actuator.throttle = 0.25
772-
773-
assert math.isfinite(actuator._alpha)
774-
assert math.isfinite(actuator.throttle)
775-
776-
def test_a_huge_time_constant_does_not_pin_the_coefficient_to_zero(self):
777-
"""The other end of the same overflow. The denominator went infinite and
778-
alpha came out 0, which freezes the actuator at its initial value; the
779-
answer here is about 0.5."""
780-
actuator = ThrottleActuator(
781-
demand_rate=1e-308,
782-
throttle_time_constant=1e308,
783-
throttle_range=(0.0, 1.0),
784-
initial_throttle=0.5,
785-
)
786-
787-
assert actuator._alpha == pytest.approx(0.5)
788-
789704
@pytest.mark.parametrize(
790705
"demand_rate, time_constant",
791706
[(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
872787
controller = calisto._controllers[-1]
873788

874789
assert 1 / controller.sampling_rate == pytest.approx(0.01)
875-
876-
877-
class TestAFilterThatCannotRespondIsRefused:
878-
"""The rewritten coefficient removed one overflow and left a second.
879-
880-
``tau * demand_rate`` can overflow where neither factor does, and ``1 / inf``
881-
is 0, which is a filter that never moves. Measured: an actuator built that
882-
way holds its initial output against every command for the whole flight.
883-
"""
884-
885-
@pytest.mark.parametrize(
886-
"time_constant, demand_rate", [(1e200, 1e200), (1e308, 1e10), (1e154, 1e155)]
887-
)
888-
def test_a_coefficient_that_overflows_to_zero_is_refused(
889-
self, time_constant, demand_rate
890-
):
891-
with pytest.raises(ValueError, match="cannot respond"):
892-
ThrottleActuator(
893-
demand_rate=demand_rate,
894-
throttle_time_constant=time_constant,
895-
throttle_range=(0.0, 1.0),
896-
initial_throttle=0.5,
897-
)
898-
899-
@pytest.mark.parametrize(
900-
"time_constant, demand_rate, expected",
901-
[(0.01, 100.0, 0.5), (10.0, 1000.0, 1e-4)],
902-
)
903-
def test_a_slow_actuator_is_still_allowed(
904-
self, time_constant, demand_rate, expected
905-
):
906-
"""The half that stops this being satisfied by refusing slow actuators.
907-
A small coefficient is what a slow actuator is; only zero is broken."""
908-
actuator = ThrottleActuator(
909-
demand_rate=demand_rate,
910-
throttle_time_constant=time_constant,
911-
throttle_range=(0.0, 1.0),
912-
)
913-
914-
assert actuator._alpha == pytest.approx(expected, rel=1e-3)

0 commit comments

Comments
 (0)