From d363bfa86d183249c798adefe6be7fc7442fe0c2 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 8 Aug 2026 10:58:49 +0800 Subject: [PATCH 1/5] BUG: accept the callable parachute triggers the docstring promises The `or` sat inside the isinstance call rather than beside it: isinstance(member, (str, int, float) or callable(member)) A non-empty type tuple is truthy, so the expression short-circuited to the tuple and callable(member) was never evaluated. The check reduced to isinstance(member, (str, int, float)), and a callable is none of those. Parachute takes a callable trigger and Flight calls it, and the docstring three lines above says "a list of callables, string 'apogee' or ints/floats". Only the stochastic wrapper refused one. The two non-callable forms passed throughout, which is why the tests never caught it. Left as an assert to match the other fourteen in these two modules, and because raising a different type would break anyone catching AssertionError. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- rocketpy/stochastic/stochastic_parachute.py | 5 ++- .../stochastic/test_stochastic_parachute.py | 36 +++++++++++++++++++ 2 files changed, 40 insertions(+), 1 deletion(-) diff --git a/rocketpy/stochastic/stochastic_parachute.py b/rocketpy/stochastic/stochastic_parachute.py index 038907187..29e7443db 100644 --- a/rocketpy/stochastic/stochastic_parachute.py +++ b/rocketpy/stochastic/stochastic_parachute.py @@ -120,8 +120,11 @@ def _validate_trigger(self, trigger): - a tuple that will be further validated in the StochasticModel class """ if trigger is not None: + # The `or` belongs beside the isinstance, not inside it. A non-empty + # type tuple is truthy, so `X or callable(member)` short-circuited to + # X and the callables this docstring promises were refused. assert isinstance(trigger, list) and all( - isinstance(member, (str, int, float) or callable(member)) + isinstance(member, (str, int, float)) or callable(member) for member in trigger ), "`trigger` must be a list of callables, string 'apogee' or ints/floats" diff --git a/tests/unit/stochastic/test_stochastic_parachute.py b/tests/unit/stochastic/test_stochastic_parachute.py index 09a1497f7..d16af85ef 100644 --- a/tests/unit/stochastic/test_stochastic_parachute.py +++ b/tests/unit/stochastic/test_stochastic_parachute.py @@ -1,3 +1,6 @@ +import pytest + +from rocketpy.stochastic import StochasticParachute from rocketpy.rocket.parachute import Parachute @@ -19,3 +22,36 @@ 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, [None], [{}]], ids=str) +def test_a_trigger_that_is_not_a_list_of_those_is_still_refused( + calisto_main_chute, trigger +): + """The control. Moving the `or` must not turn the check into one that + accepts anything: a bare callable is not a list, and None is none of the + three.""" + with pytest.raises(AssertionError, match="must be a list"): + StochasticParachute(calisto_main_chute, trigger=trigger) From ba9b5433d7d0d7ec0dbf27c40f364b1bacb00d51 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 8 Aug 2026 12:40:27 +0800 Subject: [PATCH 2/5] BUG: refuse the trigger forms Parachute cannot use Moving the `or` let callables through, but left four things the check should never have accepted. `["banana"]` passed here and then `Parachute` raised ValueError, so the wrapper only moved the failure to create time. `[True]` is worse: bool is an int, so it went through as a height of one metre. `[]` passed because `all([])` is True. And `numbers.Real` replaces `(int, float)`, which took numpy.float64 because it subclasses float and refused numpy.int64 because it subclasses neither. The check is raised rather than asserted. `python -O` strips an assert outright, and this is the only thing between those triggers and a Parachute that either refuses them later or misreads them. Still an AssertionError, so nothing that catches it has to change. The docstring claimed a tuple form that was never implemented; it now describes what the code does. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- rocketpy/stochastic/stochastic_parachute.py | 47 +++++++++--- .../stochastic/test_stochastic_parachute.py | 74 +++++++++++++++++-- 2 files changed, 101 insertions(+), 20 deletions(-) diff --git a/rocketpy/stochastic/stochastic_parachute.py b/rocketpy/stochastic/stochastic_parachute.py index 29e7443db..838914975 100644 --- a/rocketpy/stochastic/stochastic_parachute.py +++ b/rocketpy/stochastic/stochastic_parachute.py @@ -1,10 +1,26 @@ """Defines the StochasticParachute class.""" +from numbers import Real + from rocketpy.rocket import Parachute from .stochastic_model import StochasticModel +def _is_a_trigger(member): + """One of the three forms ``Parachute`` accepts. + + ``Real`` rather than ``(int, float)``, which took ``numpy.float64`` but not + ``numpy.int64``. ``bool`` is excluded because it is an ``int``, and + ``Parachute`` would read ``True`` as a height of one metre. + """ + if callable(member): + return True + if isinstance(member, str): + return member.lower() == "apogee" + return isinstance(member, Real) and not isinstance(member, bool) + + class StochasticParachute(StochasticModel): """A Stochastic Parachute class that inherits from StochasticModel. @@ -114,19 +130,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: - # The `or` belongs beside the isinstance, not inside it. A non-empty - # type tuple is truthy, so `X or callable(member)` short-circuited to - # X and the callables this docstring promises were refused. - 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 d16af85ef..2362ff9d0 100644 --- a/tests/unit/stochastic/test_stochastic_parachute.py +++ b/tests/unit/stochastic/test_stochastic_parachute.py @@ -1,3 +1,6 @@ +import inspect + +import numpy as np import pytest from rocketpy.stochastic import StochasticParachute @@ -46,12 +49,67 @@ def test_every_documented_trigger_form_is_accepted(calisto_main_chute, trigger): StochasticParachute(calisto_main_chute, trigger=trigger) -@pytest.mark.parametrize("trigger", [_at_apogee, "apogee", 800, [None], [{}]], ids=str) -def test_a_trigger_that_is_not_a_list_of_those_is_still_refused( - calisto_main_chute, trigger -): - """The control. Moving the `or` must not turn the check into one that - accepts anything: a bare callable is not a list, and None is none of the - three.""" - with pytest.raises(AssertionError, match="must be a list"): +@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", [800, 800.0, np.float64(800), np.int64(800)], ids=str +) +def test_a_height_is_a_height_whatever_numeric_type_it_arrives_as( + calisto_main_chute, member +): + """`(int, float)` accepted `numpy.float64`, which subclasses `float`, and + refused `numpy.int64`, which subclasses neither.""" + 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 + descending = [0.0] * 5 + [-5.0] + [0.0] * 8 + ascending = [0.0] * 5 + [5.0] + [0.0] * 8 + assert built.triggerfunc(0.0, 100.0, descending, [], []) + assert not built.triggerfunc(0.0, 100.0, ascending, [], []) From 0b21b2e0db26ffb3058fb3127a3ed4e2234e4948 Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 8 Aug 2026 13:01:09 +0800 Subject: [PATCH 3/5] BUG: do not accept numpy integers the Parachute will refuse Widening the height check to numbers.Real was the same mistake as accepting "banana": Parachute checks isinstance(trigger, (int, float)), so numpy.float64 passes because it subclasses float and numpy.int64 raises ValueError because it subclasses neither. Letting them through here only moved the failure to create_object. Back to (int, float), matching that check rather than improving on it, and the test that asserted numpy.int64 was accepted now asserts both ends refuse it. The asymmetry is Parachute's rather than this wrapper's and is worth fixing there, where widening the check would not strand anything downstream. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- rocketpy/stochastic/stochastic_parachute.py | 13 ++++---- .../stochastic/test_stochastic_parachute.py | 30 +++++++++++++++---- 2 files changed, 31 insertions(+), 12 deletions(-) diff --git a/rocketpy/stochastic/stochastic_parachute.py b/rocketpy/stochastic/stochastic_parachute.py index 838914975..787a98cd7 100644 --- a/rocketpy/stochastic/stochastic_parachute.py +++ b/rocketpy/stochastic/stochastic_parachute.py @@ -1,24 +1,23 @@ """Defines the StochasticParachute class.""" -from numbers import Real - from rocketpy.rocket import Parachute from .stochastic_model import StochasticModel def _is_a_trigger(member): - """One of the three forms ``Parachute`` accepts. + """One of the three forms ``Parachute`` accepts, and no more. - ``Real`` rather than ``(int, float)``, which took ``numpy.float64`` but not - ``numpy.int64``. ``bool`` is excluded because it is an ``int``, and - ``Parachute`` would read ``True`` as a height of one metre. + ``(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, Real) and not isinstance(member, bool) + return isinstance(member, (int, float)) and not isinstance(member, bool) class StochasticParachute(StochasticModel): diff --git a/tests/unit/stochastic/test_stochastic_parachute.py b/tests/unit/stochastic/test_stochastic_parachute.py index 2362ff9d0..1c8e94a5b 100644 --- a/tests/unit/stochastic/test_stochastic_parachute.py +++ b/tests/unit/stochastic/test_stochastic_parachute.py @@ -78,14 +78,34 @@ def test_a_trigger_that_is_not_a_list_of_those_is_refused(calisto_main_chute, tr @pytest.mark.parametrize( - "member", [800, 800.0, np.float64(800), np.int64(800)], ids=str + "member", + [_at_apogee, "apogee", "APOGEE", 800, 800.0, np.float64(800)], + ids=str, ) -def test_a_height_is_a_height_whatever_numeric_type_it_arrives_as( +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 ): - """`(int, float)` accepted `numpy.float64`, which subclasses `float`, and - refused `numpy.int64`, which subclasses neither.""" - StochasticParachute(calisto_main_chute, trigger=[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(): From 71a6688eed6e6448d97931f33656bc0cdfc9f5cc Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 8 Aug 2026 13:50:28 +0800 Subject: [PATCH 4/5] TST: use the state length Flight actually passes The trigger tests built a 14-element state. Flight passes 13: x y z vx vy vz e0 e1 e2 e3 wx wy wz. Only y[5] is read, so both worked, but the test is there to document the contract and was documenting it wrongly. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- tests/unit/stochastic/test_stochastic_parachute.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/tests/unit/stochastic/test_stochastic_parachute.py b/tests/unit/stochastic/test_stochastic_parachute.py index 1c8e94a5b..8fc128f54 100644 --- a/tests/unit/stochastic/test_stochastic_parachute.py +++ b/tests/unit/stochastic/test_stochastic_parachute.py @@ -129,7 +129,8 @@ def test_a_callable_trigger_reaches_the_parachute_and_gets_called( built = stochastic.create_object() assert built.trigger is _at_apogee - descending = [0.0] * 5 + [-5.0] + [0.0] * 8 - ascending = [0.0] * 5 + [5.0] + [0.0] * 8 + # 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, [], []) From 8b165129755409003625fb8814734f5baa98d95d Mon Sep 17 00:00:00 2001 From: thc1006 <84045975+thc1006@users.noreply.github.com> Date: Sat, 8 Aug 2026 17:23:43 +0800 Subject: [PATCH 5/5] DOC: add the changelog entry for this branch By hand, because the automation cannot run on a pull request from a fork (#1101). No documented API breaks, but the observable behaviour does: an invalid string, an empty list or a boolean trigger now fails during StochasticParachute validation rather than later in Parachute construction, or silently becoming a one-metre height trigger. Signed-off-by: thc1006 <84045975+thc1006@users.noreply.github.com> --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) 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