From 7bb58b784e96721263b7457c69e68958fd4a3b30 Mon Sep 17 00:00:00 2001 From: abhi-0203 Date: Sun, 9 Aug 2026 04:32:18 +0000 Subject: [PATCH 1/2] fix: accept numpy integer types as Parachute trigger isinstance(trigger, (int, float)) rejects numpy integer types (np.int64, np.int32) because they don't subclass Python's int or float. Replace with isinstance(trigger, numbers.Real) which covers all numeric types (int, float, numpy scalars) while excluding bool. Fixes #1106 --- rocketpy/rocket/parachute.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/rocketpy/rocket/parachute.py b/rocketpy/rocket/parachute.py index d56a24b63..a5414005f 100644 --- a/rocketpy/rocket/parachute.py +++ b/rocketpy/rocket/parachute.py @@ -1,4 +1,5 @@ from inspect import Parameter, signature +from numbers import Real import numpy as np @@ -363,7 +364,7 @@ def wrapper(p, h, y, sensors, u_dot): return # Numeric altitude trigger - if isinstance(trigger, (int, float)): + if isinstance(trigger, Real) and not isinstance(trigger, bool): self._trigger_falling_only = True def triggerfunc(p, h, y, sensors, u_dot): # pylint: disable=unused-argument From 4a29ccb4d1a9b2d4cc79bcb46825305d7438d2bd Mon Sep 17 00:00:00 2001 From: Gui-FernandesBR Date: Sat, 15 Aug 2026 12:30:34 -0300 Subject: [PATCH 2/2] BUG: accept a NumPy integer height in stochastic/ as well This PR widened Parachute's height check from `(int, float)` to `numbers.Real`, so a height read out of a NumPy array is accepted. StochasticParachute validates the same triggers before a Parachute is ever built, and its copy of the check was still spelled `(int, float)`. A `numpy.int64` height was therefore still refused there, even though the Parachute it would have built accepts it. That mismatch is what turned all six Pytest legs red: `test_a_numpy_integer_is_refused_here_because_parachute_refuses_it` pinned the old asymmetry, and its own docstring said the fix belonged in Parachute. Rather than restate the predicate a second time, Parachute now exposes it as `_is_a_height_trigger` and stochastic/ calls that. The two spellings drifted apart once already; sharing one definition is what stops it happening again. Tests: - the NumPy integers move into `test_what_this_accepts_is_what_a_parachute_accepts`, where they now belong, joined by `numpy.float32` - the refusal test is rewritten around what is still refused by both: `numpy.bool_` and the complex types, none of which are `Real` - a new test asserts the agreement itself over the whole boundary, so changing one side alone fails with both verdicts printed - Parachute gains boundary tests of its own, including that `True` is still refused rather than read as a height of one metre Verified: reverting either side alone turns these red (the drift guard reports `np.float32(800.0): stochastic/ says False, Parachute says True`); ruff check and format clean; pylint 10.00/10; tests/unit 2098 passed, 16 skipped. Co-Authored-By: Claude Opus 5 (1M context) --- rocketpy/rocket/parachute.py | 23 ++++- rocketpy/stochastic/stochastic_parachute.py | 13 +-- tests/unit/rocket/test_parachute.py | 40 +++++++++ .../stochastic/test_stochastic_parachute.py | 84 ++++++++++++++++--- 4 files changed, 141 insertions(+), 19 deletions(-) diff --git a/rocketpy/rocket/parachute.py b/rocketpy/rocket/parachute.py index a5414005f..1efc66d34 100644 --- a/rocketpy/rocket/parachute.py +++ b/rocketpy/rocket/parachute.py @@ -9,6 +9,27 @@ from ..prints.parachute_prints import _ParachutePrints +def _is_a_height_trigger(trigger): + """Whether ``trigger`` is a number this class will read as a height. + + ``numbers.Real`` rather than ``(int, float)`` so that NumPy scalars are + accepted: ``numpy.float64`` happens to subclass ``float``, but + ``numpy.int64`` and ``numpy.float32`` subclass neither and were refused + even though every arithmetic use of them here works. + + What that spelling leaves out is what should be left out. ``numpy.bool_`` + and the complex types are not ``Real``, so they still fall through to the + error. ``bool`` is excluded by hand because it *is* an ``int``, and ``True`` + would otherwise be taken as a height of one metre. + + This is the single definition of the height form. ``StochasticParachute`` + validates the same triggers before a ``Parachute`` is ever built and calls + this rather than restating it, because the two spellings drifted apart once + already. + """ + return isinstance(trigger, Real) and not isinstance(trigger, bool) + + class Parachute: """Keeps information of the parachute, which is modeled as a hemispheroid. @@ -364,7 +385,7 @@ def wrapper(p, h, y, sensors, u_dot): return # Numeric altitude trigger - if isinstance(trigger, Real) and not isinstance(trigger, bool): + if _is_a_height_trigger(trigger): self._trigger_falling_only = True def triggerfunc(p, h, y, sensors, u_dot): # pylint: disable=unused-argument diff --git a/rocketpy/stochastic/stochastic_parachute.py b/rocketpy/stochastic/stochastic_parachute.py index c0c49298c..bda6446b4 100644 --- a/rocketpy/stochastic/stochastic_parachute.py +++ b/rocketpy/stochastic/stochastic_parachute.py @@ -1,6 +1,7 @@ """Defines the StochasticParachute class.""" from rocketpy.rocket import Parachute +from rocketpy.rocket.parachute import _is_a_height_trigger from .stochastic_model import StochasticModel, _sampler_seed @@ -8,16 +9,18 @@ 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. + The height form defers to ``Parachute``'s own predicate instead of + restating it. Both were written out separately before and drifted: this one + kept ``(int, float)`` while ``Parachute`` widened to ``numbers.Real``, so a + ``numpy.int64`` height was refused here even though the ``Parachute`` it + would have built accepts it. Calling the same function is what keeps the + promise that what this accepts is what a parachute accepts. """ if callable(member): return True if isinstance(member, str): return member.lower() == "apogee" - return isinstance(member, (int, float)) and not isinstance(member, bool) + return _is_a_height_trigger(member) class StochasticParachute(StochasticModel): diff --git a/tests/unit/rocket/test_parachute.py b/tests/unit/rocket/test_parachute.py index 7a61c2349..c40dd5a5c 100644 --- a/tests/unit/rocket/test_parachute.py +++ b/tests/unit/rocket/test_parachute.py @@ -130,3 +130,43 @@ def test_callable_trigger_arities_route_arguments(trigger, expects_udot): result = parachute.triggerfunc(800.0, 500.0, [0.0] * 6, [], [1.0] * 6) assert result is True assert parachute.triggerfunc._expects_udot is expects_udot + + +@pytest.mark.parametrize( + "trigger", + [800, 800.0, np.int64(800), np.int32(800), np.float64(800), np.float32(800)], + ids=str, +) +def test_any_real_number_is_read_as_a_height(trigger): + """A height is anything ``numbers.Real``, not just ``int`` and ``float``. + + The check used to be ``isinstance(trigger, (int, float))``. ``numpy.float64`` + subclasses ``float`` and passed, but ``numpy.int64`` and ``numpy.float32`` + subclass neither, so a height read out of a NumPy array raised even though + it compares and arithmetics exactly like the value that worked.""" + parachute = _make_parachute(trigger=trigger) + + # Truthiness rather than `is True`: comparing against a NumPy scalar gives + # back a numpy.bool_, which is not the `True` singleton. + # falling (vz < 0) and below the trigger height + assert parachute.triggerfunc(0.0, 700.0, [0.0] * 5 + [-1.0], [], None) + # falling but still above it + assert not parachute.triggerfunc(0.0, 900.0, [0.0] * 5 + [-1.0], [], None) + # below it but still ascending + assert not parachute.triggerfunc(0.0, 700.0, [0.0] * 5 + [1.0], [], None) + + +@pytest.mark.parametrize( + "trigger", + [True, False, np.bool_(True), complex(800), np.complex64(800), "banana", None, {}], + ids=str, +) +def test_what_is_not_a_height_is_still_refused(trigger): + """Widening to ``numbers.Real`` must not turn the check into "anything". + + ``bool`` is the one that has to be excluded by hand, because it *is* an + ``int``: ``True`` would otherwise be accepted and read as a height of one + metre, firing the parachute a metre above the ground. ``numpy.bool_`` and + the complex types need no special case, since neither is ``Real``.""" + with pytest.raises(ValueError, match="Unable to set the trigger"): + _make_parachute(trigger=trigger) diff --git a/tests/unit/stochastic/test_stochastic_parachute.py b/tests/unit/stochastic/test_stochastic_parachute.py index 8fc128f54..e444a2ae7 100644 --- a/tests/unit/stochastic/test_stochastic_parachute.py +++ b/tests/unit/stochastic/test_stochastic_parachute.py @@ -5,6 +5,7 @@ from rocketpy.stochastic import StochasticParachute from rocketpy.rocket.parachute import Parachute +from rocketpy.stochastic.stochastic_parachute import _is_a_trigger def test_stochastic_parachute_create_object(stochastic_main_parachute): @@ -79,28 +80,47 @@ def test_a_trigger_that_is_not_a_list_of_those_is_refused(calisto_main_chute, tr @pytest.mark.parametrize( "member", - [_at_apogee, "apogee", "APOGEE", 800, 800.0, np.float64(800)], + [ + _at_apogee, + "apogee", + "APOGEE", + 800, + 800.0, + np.float64(800), + np.float32(800), + np.int64(800), + np.int32(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.""" + has to survive `Parachute`, or the check has only moved the failure. + + The NumPy integers used to belong to the test below, refused by both + because `Parachute` spelled its height check `(int, float)`: `numpy.float64` + subclasses `float` and passed, `numpy.int64` subclasses neither and raised. + `Parachute` now reads a height as `numbers.Real`, so they are heights like + any other and belong here.""" 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. - """ +@pytest.mark.parametrize( + "member", + [np.bool_(True), complex(800), np.complex64(800), "banana", None, {}], + ids=str, +) +def test_what_this_refuses_is_what_a_parachute_refuses(calisto_main_chute, member): + """The other half of the same property, and the half that keeps the widened + height check honest. + + `numbers.Real` was the wider spelling, but not an unbounded one: neither + `numpy.bool_` nor the complex types are `Real`, so they still reach the + error rather than being read as a height. `numpy.bool_` needs no exclusion + of its own for the same reason -- unlike `bool`, which is an `int` and is + ruled out by hand.""" with pytest.raises(ValueError, match="Unable to set the trigger"): Parachute("probe", 10.0, member, 105, 1.5) @@ -108,6 +128,44 @@ def test_a_numpy_integer_is_refused_here_because_parachute_refuses_it( StochasticParachute(calisto_main_chute, trigger=[member]) +def test_neither_check_can_drift_from_the_other_again(): + """The two checks were written out separately and disagreed: a + `numpy.int64` height was refused in `stochastic/` and accepted by the + `Parachute` that would have been built from it. Nothing failed, because + each side had a test asserting its own half. + + They now share one predicate, so this asserts the agreement itself over the + whole boundary rather than a list of types on either side.""" + boundary = [ + 800, + 800.0, + np.float64(800), + np.float32(800), + np.int64(800), + np.int32(800), + True, + np.bool_(True), + complex(800), + np.complex64(800), + "apogee", + "banana", + None, + ] + + for member in boundary: + try: + Parachute("probe", 10.0, member, 105, 1.5) + except ValueError: + parachute_accepts = False + else: + parachute_accepts = True + + assert _is_a_trigger(member) is parachute_accepts, ( + f"{member!r}: stochastic/ says {_is_a_trigger(member)}, " + f"Parachute says {parachute_accepts}" + ) + + 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