diff --git a/rocketpy/rocket/parachute.py b/rocketpy/rocket/parachute.py index d56a24b63..1efc66d34 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 @@ -8,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. @@ -363,7 +385,7 @@ def wrapper(p, h, y, sensors, u_dot): return # Numeric altitude trigger - if isinstance(trigger, (int, float)): + 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