BUG: accept the callable parachute triggers the docstring promises

[thc1006]

Addresses #1095. Not Closes, because the keyword only fires when a pull request targets the default branch and this targets develop.

Pull request type

• Code changes (bugfix, features)

Checklist

• Tests for the changes have been added
• Lint (ruff check / ruff format --check / pylint rocketpy/ tests/ docs/) has passed locally
• All tests have passed locally

pylint exits 0, pytest tests/unit tests/integration is 2058 passed, 44 skipped.

Current behavior

StochasticParachute._validate_trigger refused the callable triggers its own docstring promised:

isinstance(member, (str, int, float) or callable(member))

The or sat inside the isinstance call. (str, int, float) is a non-empty tuple and therefore 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.

Fixing that turned up four more things the check should never have accepted:

["banana"]   passed here, then Parachute raised ValueError at create time
[True]       passed as a height of one metre, since bool is an int
[]           passed, because all([]) is True
numpy.int64  refused, while numpy.float64 was accepted

New behavior

The check now accepts exactly what Parachute accepts, and nothing else:

def _is_a_trigger(member):
    if callable(member):
        return True
    if isinstance(member, str):
        return member.lower() == "apogee"
    return isinstance(member, (int, float)) and not isinstance(member, bool)

(int, float) rather than numbers.Real, deliberately. Parachute uses that exact check, so widening here would let numpy.int64 past validation and into create_object, which is the same defect as accepting "banana": not permissiveness, but moving the failure somewhere less obvious. bool is excluded because Parachute would otherwise read True as one metre without complaint.

The whole contract, checked against both sides in one test:

value              Parachute   this check
callable           accepts     accepts
"apogee"/"APOGEE"  accepts     accepts
"banana"           refuses     refuses
800 / 800.0        accepts     accepts
numpy.float64      accepts     accepts
numpy.float32      refuses     refuses
numpy.int64        refuses     refuses
None / {} / ()     refuses     refuses
True / False       accepts     refuses

The last row is the one deliberate gap, in the safe direction.

The check is also raised from an if rather than asserted. It stays an AssertionError, so nothing catching it has to change, but 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.

The docstring promised a tuple form that was never implemented, and now describes what the code does.

Tests

Twenty-five. Five mutations, one per rule, each taking only its own case:

mutation	fails
callable branch removed	the callable and mixed cases
accept any string	["banana"]
drop the bool exclusion	[True]
drop the non-empty check	[]
(int, float) widened to numbers.Real	numpy.int64
raise back to assert	the -O test

There is also the end-to-end case that was missing: a callable has to survive create_object and be what gets called, rather than merely be accepted by the constructor.

built.trigger is _at_apogee
triggerfunc(0.0, 100.0, descending, [], [])  -> True
triggerfunc(0.0, 100.0, ascending,  [], [])  -> False

Breaking change

• No

It accepts callables, which the docstring already promised, and refuses four forms that either failed later or were silently misread.

Additional information

Parachute's own numeric check is asymmetric: numpy.float64 passes because it subclasses float, numpy.int64 does not because it subclasses neither, and True is taken as a height. This follows that contract rather than trying to improve on it, and the asymmetry is filed separately as #1106, where widening would not strand anything downstream.

Found while reviewing #1054, and unrelated to it.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 82.57%. Comparing base (e0ff281) to head (8b16512).
⚠️ Report is 24 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #1103      +/-   ##
===========================================
+ Coverage    82.18%   82.57%   +0.39%     
===========================================
  Files          122      128       +6     
  Lines        16355    16564     +209     
===========================================
+ Hits         13441    13678     +237     
+ Misses        2914     2886      -28     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[thc1006]

All four taken, in ba9b543. Each one reproduced before I touched it, and one of them is worse than the review says.

Strings. ["banana"] passed this check and then Parachute.__init__ raised ValueError. So the wrapper was not being permissive, it was moving the failure to create time, which is the opposite of what a validator is for. Only "apogee" now, case-insensitively, since Parachute accepts APOGEE and Apogee too.

Booleans. This is the one I would call a real bug rather than a loose check. bool is an int, so [True] passed and Parachute took it as a height of one metre. No error anywhere, just a parachute that deploys at 1 m.

Empty list. all([]) is True, so [] passed here and failed later.

Numeric types. While checking the above I found (int, float) was inconsistent rather than merely narrow:

[numpy.float64(800)]   accepted     float64 subclasses float
[numpy.int64(800)]     refused      int64 subclasses neither

numbers.Real takes both, with bool excluded explicitly.

python -O. Kept as AssertionError so nothing catching it has to change, but raised from an if rather than asserted. A test reads the source and fails if an assert statement comes back, since the behaviour it guards is only visible under -O.

The docstring. It promised "a tuple that will be further validated in the StochasticModel class". No tuple has ever reached that path; the check has always required a list. Rewritten to say what the code does, and (800,) is in the refusal list.

Tests

21 now. Five mutations, one per rule, each taking only its own case:

mutation	fails
drop the bool exclusion	[True]
accept any string	["banana"]
drop the non-empty check	[]
numbers.Real back to (int, float)	numpy.int64(800)
raise back to assert	the -O test

There is also the end-to-end case that was missing: the callable now has to survive create_object and be what gets called, not merely be accepted by the constructor.

built.trigger is _at_apogee
triggerfunc(0.0, 100.0, descending, [], [])  -> True
triggerfunc(0.0, 100.0, ascending,  [], [])  -> False

Local: ruff clean, pylint rocketpy/ tests/ docs/ exit 0, 2054 passed and 44 skipped.

[thc1006]

Good catch, and it is the same mistake I spent the last round criticising. Fixed in 0b21b2e.

I widened the height check to numbers.Real because (int, float) looked arbitrarily narrow. Parachute uses that exact check, and the two numpy types land on opposite sides of it:

numpy.float64(800)   accepted    subclasses float
numpy.int64(800)     ValueError  subclasses neither

So numbers.Real let numpy.int64 past the wrapper and into create_object, where Parachute raised. That is precisely what I said was wrong about accepting "banana": not permissiveness, but moving the failure to a later and less obvious place. I wrote the principle down in the same commit that broke it.

The test made it worse. test_a_height_is_a_height_whatever_numeric_type_it_arrives_as asserted numpy.int64 was accepted, so it was pinning the defect rather than catching it.

Back to (int, float) now, matching Parachute rather than improving on it, and the tests are rewritten around the property instead of a list of types:

• what the wrapper accepts, Parachute also accepts, asserted on both in the same test
• numpy.int64 and numpy.int32 are refused at both ends, with the reason in the docstring

Putting it back to numbers.Real now fails those.

The asymmetry itself is real and is Parachute's rather than this wrapper's, so I have filed it as #1106 instead of carrying it here. A height read out of an array or a pandas-parsed config arrives as numpy.int64 whenever it has no decimal point, so it is reachable without anyone reaching for a numpy scalar deliberately. Widening the check there would let both ends agree, and this wrapper follows whatever that one decides.

Local: ruff clean, pylint rocketpy/ tests/ docs/ exit 0, 2058 passed and 44 skipped.

[thc1006]

This was true, and it is what 0b21b2e fixed a few hours ago. Since the two contracts are easier to check than to argue about, here is the whole table on the current head rather than the numeric row alone:

value              Parachute   wrapper
callable           accepts     accepts
"apogee"           accepts     accepts
"APOGEE"           accepts     accepts
"banana"           refuses     refuses
800                accepts     accepts
800.0              accepts     accepts
numpy.float64      accepts     accepts
numpy.float32      refuses     refuses
numpy.int64        refuses     refuses
numpy.int32        refuses     refuses
None               refuses     refuses
{}                 refuses     refuses
True               accepts     refuses
False              accepts     refuses

Twelve of fourteen agree. The two that do not are True and False, and that gap is on purpose: bool is an int, so Parachute takes True as a height of one metre without complaint. Refusing it in the wrapper is the safe direction, and a test pins it.

What is left is the asymmetry inside Parachute itself, which takes numpy.float64 because it subclasses float and refuses numpy.int64 because it subclasses neither. The wrapper now mirrors that rather than trying to improve on it, so the two are consistently narrow instead of inconsistently so. Widening belongs in Parachute, where nothing downstream would be stranded by it, and that is #1106.

One smaller thing from the same round, in 71a6688e: the trigger tests built a 14-element state and Flight passes 13. Only y[5] is read so both worked, but a test whose job is to document the contract should not misstate it.

[thc1006]

Added the changelog entry here rather than waiting on the automation, which cannot run on a fork pull request until #1101 is fixed. Head is 8b165129.

Your wording on the scope is better than mine, so the entry follows it: no documented API break, but an invalid string, an empty list or a boolean now fails during StochasticParachute validation instead of reaching Parachute construction or silently becoming a one-metre height trigger.