Skip to content

Commit b097411

Browse files
committed
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>
1 parent 4ecd10f commit b097411

2 files changed

Lines changed: 60 additions & 3 deletions

File tree

rocketpy/rocket/actuator/actuator.py

Lines changed: 20 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -79,9 +79,13 @@ def _range_or_raise(value, description):
7979
upper = float(upper)
8080
except (TypeError, ValueError) as error:
8181
raise ValueError(f"{description} endpoints must be numbers.") from error
82-
# Written as a positive test because NaN fails every ordered comparison, so
83-
# a range of two NaNs would slip through a check phrased as a negation.
84-
if not lower <= upper:
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:
8589
raise ValueError(f"{description}[0] must be <= {description}[1].")
8690
if lower == np.inf or upper == -np.inf:
8791
raise ValueError(
@@ -210,6 +214,19 @@ def _update_iir_coefficients(self):
210214
self._alpha = 1.0 / (
211215
1.0 + self.actuator_time_constant * self.demand_rate
212216
)
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+
)
213230
else:
214231
warnings.warn(
215232
f"Actuator time constant currently only implemented on discrete controllers. '{self.name}' dynamics not applied."

tests/unit/rocket/test_actuators.py

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -872,3 +872,43 @@ def test_the_controller_rate_is_usable_as_a_time_step(self, calisto, adder, extr
872872
controller = calisto._controllers[-1]
873873

874874
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)