diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ab28dd89..dc36de12b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -43,6 +43,7 @@ Attention: The newest changes should be on top --> ### Fixed +- BUG: Accept the callable parachute triggers `StochasticParachute` documents, and reject the ones it cannot mean. An invalid string, an empty list or a boolean now fails during validation instead of reaching `Parachute` or becoming a one-metre height trigger. [#1103](https://github.com/RocketPy-Team/RocketPy/pull/1103) - BUG: rocket with a late-starting thrust curve never leaves the rail [#1085](https://github.com/RocketPy-Team/RocketPy/pull/1085) ## [v1.13.0] - 2026-07-21 diff --git a/rocketpy/stochastic/stochastic_parachute.py b/rocketpy/stochastic/stochastic_parachute.py index 038907187..787a98cd7 100644 --- a/rocketpy/stochastic/stochastic_parachute.py +++ b/rocketpy/stochastic/stochastic_parachute.py @@ -5,6 +5,21 @@ from .stochastic_model import StochasticModel +def _is_a_trigger(member): + """One of the three forms ``Parachute`` accepts, and no more. + + ``(int, float)`` deliberately, matching ``Parachute``'s own check rather + than ``numbers.Real``: that would take ``numpy.int64``, which ``Parachute`` + refuses, so widening here only moves the failure to create time. ``bool`` + is excluded because it is an ``int``, and would arrive as a height of one. + """ + if callable(member): + return True + if isinstance(member, str): + return member.lower() == "apogee" + return isinstance(member, (int, float)) and not isinstance(member, bool) + + class StochasticParachute(StochasticModel): """A Stochastic Parachute class that inherits from StochasticModel. @@ -114,16 +129,26 @@ def __init__( ) def _validate_trigger(self, trigger): - """Validates the trigger input. If the trigger input argument is not - None, it must be: - - a list of callables, string "apogee" or ints/floats - - a tuple that will be further validated in the StochasticModel class + """Validates the trigger input. If not None, it must be a non-empty + list whose members are each a callable, the string "apogee", or a + height. One of those is chosen per simulation. """ - if trigger is not None: - assert isinstance(trigger, list) and all( - isinstance(member, (str, int, float) or callable(member)) - for member in trigger - ), "`trigger` must be a list of callables, string 'apogee' or ints/floats" + if trigger is None: + return + + valid = ( + isinstance(trigger, list) + and bool(trigger) + and all(_is_a_trigger(member) for member in trigger) + ) + # Raised rather than asserted: `python -O` strips an assert, and this + # is the only thing standing between a bad trigger and a Parachute + # that either refuses it much later or reads True as a height of 1. + if not valid: + raise AssertionError( + "`trigger` must be a non-empty list whose members are " + "callables, the string 'apogee', or heights" + ) def _validate_noise(self, noise): """Validates the noise input. If the noise input argument is not diff --git a/tests/unit/stochastic/test_stochastic_parachute.py b/tests/unit/stochastic/test_stochastic_parachute.py index 09a1497f7..8fc128f54 100644 --- a/tests/unit/stochastic/test_stochastic_parachute.py +++ b/tests/unit/stochastic/test_stochastic_parachute.py @@ -1,3 +1,9 @@ +import inspect + +import numpy as np +import pytest + +from rocketpy.stochastic import StochasticParachute from rocketpy.rocket.parachute import Parachute @@ -19,3 +25,112 @@ class creates a StochasticParachute object from the randomly generated """ obj = stochastic_main_parachute.create_object() assert isinstance(obj, Parachute) + + +def _at_apogee(pressure, height, state): # pylint: disable=unused-argument + """A trigger of the kind `Parachute` and `Flight` already accept. + + Keeps the full signature rather than underscoring the unused two, since the + signature is the contract being tested.""" + return state[5] < 0 + + +@pytest.mark.parametrize( + "trigger", + [[_at_apogee], ["apogee"], [800], [_at_apogee, "apogee", 800]], + ids=["callable", "apogee", "height", "mixed"], +) +def test_every_documented_trigger_form_is_accepted(calisto_main_chute, trigger): + """The docstring promises callables, "apogee" and numbers. The check read + `isinstance(member, (str, int, float) or callable(member))`, and a non-empty + type tuple is truthy, so the `or` short-circuited and callables were + refused. The two non-callable forms passed throughout, which is why it went + unnoticed.""" + StochasticParachute(calisto_main_chute, trigger=trigger) + + +@pytest.mark.parametrize( + "trigger", + [ + _at_apogee, + "apogee", + 800, + (800,), + [], + [None], + [{}], + ["banana"], + [True], + [_at_apogee, None], + ], + ids=str, +) +def test_a_trigger_that_is_not_a_list_of_those_is_refused(calisto_main_chute, trigger): + """The control, and four that the check used to wave through. + + `Parachute` refuses "banana" with a ValueError, so accepting it here only + moved the failure to create time. `True` is worse: it is an `int`, so it + was taken as a height of one metre. An empty list passed because `all([])` + is True. And the docstring's tuple form was never implemented. + """ + with pytest.raises(AssertionError, match="must be a non-empty list"): + StochasticParachute(calisto_main_chute, trigger=trigger) + + +@pytest.mark.parametrize( + "member", + [_at_apogee, "apogee", "APOGEE", 800, 800.0, np.float64(800)], + ids=str, +) +def test_what_this_accepts_is_what_a_parachute_accepts(calisto_main_chute, member): + """The property, rather than a list of types. Anything this lets through + has to survive `Parachute`, or the check has only moved the failure.""" + StochasticParachute(calisto_main_chute, trigger=[member]) + + Parachute("probe", 10.0, member, 105, 1.5) + + +@pytest.mark.parametrize("member", [np.int64(800), np.int32(800)], ids=str) +def test_a_numpy_integer_is_refused_here_because_parachute_refuses_it( + calisto_main_chute, member +): + """`Parachute` checks `isinstance(trigger, (int, float))`. `numpy.float64` + subclasses `float` and passes; `numpy.int64` subclasses neither and raises. + + So this check matches that one rather than `numbers.Real`, which would be + the wider and more natural spelling but would let these through to fail at + create time. The asymmetry is `Parachute`'s and is worth fixing there. + """ + with pytest.raises(ValueError, match="Unable to set the trigger"): + Parachute("probe", 10.0, member, 105, 1.5) + + with pytest.raises(AssertionError, match="must be a non-empty list"): + StochasticParachute(calisto_main_chute, trigger=[member]) + + +def test_the_check_is_not_stripped_by_python_dash_o(): + """`python -O` removes an `assert` outright, and this check is the only + thing between a bad trigger and a `Parachute` that either refuses it much + later or reads `True` as a height.""" + source = inspect.getsource(StochasticParachute._validate_trigger) + + assert "raise AssertionError" in source + assert not any(line.strip().startswith("assert ") for line in source.splitlines()) + + +def test_a_callable_trigger_reaches_the_parachute_and_gets_called( + calisto_main_chute, +): + """Constructing the wrapper is not the property that matters. The callable + has to survive `create_object` and be what `Flight` ends up calling.""" + stochastic = StochasticParachute(calisto_main_chute, trigger=[_at_apogee]) + stochastic._set_stochastic(42) + + built = stochastic.create_object() + + assert built.trigger is _at_apogee + # 13, matching the state Flight passes: x y z vx vy vz e0 e1 e2 e3 wx wy wz + descending = [0.0] * 5 + [-5.0] + [0.0] * 7 + ascending = [0.0] * 5 + [5.0] + [0.0] * 7 + assert built.triggerfunc(0.0, 100.0, descending, [], []) + assert not built.triggerfunc(0.0, 100.0, ascending, [], [])