fix: accept numpy integer types as Parachute trigger - #1116
Conversation
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 RocketPy-Team#1106
Gui-FernandesBR
left a comment
There was a problem hiding this comment.
I suggest using "NUMERICAL_TYPES" constant from the Function module
|
I exercised the trigger-type boundary on head
Using Two changes still look necessary before review:
No branch changes were made. Environment: Python 3.12.6; NumPy 2.5.2; RocketPy PR head above; macOS 26.5.2 arm64. |
|
I understand this PR still needs some fixes.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1116 +/- ##
===========================================
+ Coverage 82.18% 82.37% +0.19%
===========================================
Files 122 122
Lines 16355 16378 +23
===========================================
+ Hits 13441 13492 +51
+ Misses 2914 2886 -28 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Summary
Replace
isinstance(trigger, (int, float))withisinstance(trigger, Real) and not isinstance(trigger, bool)inParachute.__init__().numpy.int64andnumpy.int32don't subclass Python'sintorfloat, so they were rejected with aValueErroreven though they represent valid numeric altitude values.numbers.Realcovers all numeric types (int, float, numpy scalars).Also excludes
bool— currentlytrigger=Trueis silently accepted as a 1m height, which is never intentional.Changes
rocketpy/rocket/parachute.py: ImportRealfromnumbers, replaceisinstance(trigger, (int, float))withisinstance(trigger, Real) and not isinstance(trigger, bool)Test plan
int 800→ accepted ✓float 800.0→ accepted ✓np.float64(800)→ accepted ✓np.int64(800)→ accepted ✓np.int32(800)→ accepted ✓True/False→ rejected ✓ (was previously accepted as height 1m)AST verified: no old pattern remains, import confirmed.
Closes #1106