BUG/MNT: pre-release v1.13.0 review fixes#1074
Merged
Merged
Conversation
- pyproject: `rocketpy[animation]` was missing `imageio`, so GIF/MP4 export (plotter.open_gif/open_movie) raised ImportError on the documented install path (imageio is not a core pyvista dependency). - CHANGELOG: fix `rocketpy.utils.enable_logging()` -> `rocketpy.utilities` (the module is `rocketpy.utilities`; `rocketpy.utils` does not exist). - CHANGELOG: add Changed notes for the akima value change and the `Function.differentiate` analytical-derivative change (#1049). - CHANGELOG: drop the stray auto-populated #1073 meta entry. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- fit_akima raised IndexError for 2-point Functions (m[1]/m[-2] out of range); a 2-point akima now reduces to linear interpolation, restoring pre-1.13 behaviour. Adds a regression test. - fit_pchip emitted a spurious "RuntimeWarning: divide by zero" on flat segments (reciprocals evaluated before np.where masks them); wrap in np.errstate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- __determine_pressure_conversion_factor only matched GEFS/HIRESW against input_file, missing the realistic download path where the model name is input_dict; also accept "mb"/"millibar" in the units allow-list. - Environment.from_dict restored ensemble arrays but left `ensemble_member` unset (AttributeError on access); persist and restore it via to_dict. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
_make_wrapper raised TypeError for variadic triggers (e.g. `lambda *args:`, num_params < 3), a backward-compat break vs v1.12.1. Detect VAR_POSITIONAL and call such triggers with (pressure, height, state, sensors) as before. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
info() delegated to self.motor.info(), printing the single base motor's thrust/impulse instead of the N-times-scaled cluster values. Delegate to super().info() so it matches the (correct) all_info() output. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #1074 +/- ##
===========================================
+ Coverage 82.29% 82.36% +0.06%
===========================================
Files 122 122
Lines 16361 16377 +16
===========================================
+ Hits 13465 13489 +24
+ Misses 2896 2888 -8 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Pull request overview
Release-hardening fixes for the RocketPy v1.13.0 pre-release review, spanning packaging, regression/compatibility fixes, numerical robustness, and changelog updates.
Changes:
- Fix Akima interpolation for the 2-point edge case (fallback to linear) and add a regression unit test.
- Restore compatibility for variadic parachute triggers and correct a few environment serialization / pressure-unit handling paths.
- Clean up minor quality issues (PCHIP warning suppression, animation kwargs pop, ring cluster
info()output) and update release notes + optional deps.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/mathutils/test_function.py | Adds regression coverage for 2-point Akima interpolation. |
| rocketpy/mathutils/_calc/_fitting.py | Fixes 2-point Akima construction and silences harmless PCHIP divide-by-zero warnings. |
| rocketpy/rocket/parachute.py | Restores support for variadic (*args) trigger callables via signature inspection. |
| rocketpy/environment/tools.py | Expands pressure level unit auto-detection to include mb/millibar variants. |
| rocketpy/environment/environment.py | Improves model-name based pressure conversion detection and persists ensemble_member in (de)serialization. |
| rocketpy/plots/flight_plots.py | Removes redundant nested kwargs.pop(...) in animation options. |
| rocketpy/motors/ring_cluster_motor.py | Makes RingClusterMotor.info() report cluster-scaled quantities via super().info(). |
| pyproject.toml | Adds imageio to the animation extra to prevent export-time ImportError. |
| CHANGELOG.md | Cleans up an incorrect module reference and adds “Changed” notes for Function behavior updates. |
Adds tests for the new lines flagged by codecov/patch and the Copilot review on #1074: - parachute callable-trigger arities incl. variadic `*args` (argument routing and `_expects_udot`), covering the restored variadic-trigger branch. - Environment pressure factor auto-detected via `dictionary` (the realistic download path), covering the new input_dict matches. - get_pressure_levels_from_file hPa/millibar/mb/Pa unit synonyms. - ensemble_member survives the to_dict/from_dict round-trip. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…_factor Copilot review follow-up (#1074): file auto-detection accepted "mb"/"millibar", but an explicit `pressure_conversion_factor` string only accepted "mbar"/"hpa"/"pa" - so `set_atmospheric_model(..., pressure_conversion_factor= "mb")` was rejected, and "mb" would otherwise have mapped to factor 1 instead of 100. Centralise the unit->factor logic in a single `pressure_unit_to_factor` helper in environment.tools, reused by the validator, the explicit-factor mapping and file auto-detection so all three stay consistent. Adds tests. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The apogee-comparison chart is built in a jupyter-execute block that called fig.show() with no renderer configured. Under jupyter-execute (no notebook frontend) plotly emits a mimetype bundle that a static HTML page cannot display, so the chart rendered blank. Set the plotly renderer to "notebook_connected" so the output embeds a CDN plotly.js loader. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The get_pressure_levels_from_file / pressure_unit_to_factor imports were added inside the new test functions, tripping pylint's import-outside-toplevel (C0415) and failing the Linters CI job. Hoist them into the existing module-level rocketpy.environment.tools import block. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
phmbressan
approved these changes
Jul 20, 2026
thc1006
pushed a commit
to thc1006/RocketPy
that referenced
this pull request
Jul 20, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull request type
Summary
Fixes surfaced by the final pre-release code review of v1.13.0 (diff
v1.12.1..master), plus the follow-ups from the codecov/patch failure and the GitHub Copilot review on this PR. All changed areas pass their unit/integration tests locally; lint + format clean.Blocker
rocketpy[animation]was missingimageio—animate_trajectory(export_file=...)callsplotter.open_gif/open_movie, which importimageio(not a core pyvista dependency), so the documented install path raisedImportErroron every GIF/MP4 export. Addedimageioto theanimationextra.Bug fixes / regressions
Functioncrashed (IndexErrorat construction) — now reduces to linear interpolation (regression vs v1.12.1). Regression test added.lambda *args:) raisedTypeErrorinstead of running — restored pre-1.13 behaviour(pressure, height, state, sensors).input_file, missing the realistic download path (model name arrives asinput_dict); could yield pressures 100× too small. Also accept"mb"/"millibar"units.Environment.from_dictleftensemble_memberunset after decoding an ensemble model (AttributeError) — now persisted/restored viato_dict.RingClusterMotor.info()printed the single base motor's thrust/impulse instead of the N×-scaled cluster values — delegates tosuper().info().Copilot review follow-ups
ensemble_memberround-trip (also fixes thecodecov/patchgap that failed this PR).pressure_conversion_factor="mb"was rejected while files could auto-detect"mb". Centralised the unit→factor logic in a singlepressure_unit_to_factorhelper (environment.tools) reused by the validator, the explicit-factor mapping and file auto-detection.Docs
jupyter-executeblock calledfig.show()with no renderer, so plotly emitted a mimetype bundle a static HTML page can't display. Set the renderer tonotebook_connected(CDN plotly.js).Quality
fit_pchipno longer emits a spuriousRuntimeWarning: divide by zeroon flat data.kwargs.pop(...)inanimate_trajectory.rocketpy.utils→rocketpy.utilities, addedChangednotes for the akima /differentiatebehaviour changes, dropped the stray MNT: pre-release v1.13.0 cleanup — changelog consolidation + optional deps sync #1073 entry.Known follow-up (deliberately not in this PR)
Plottercleanup on error paths: the plotter created inanimate_trajectory/animate_rotateis only guaranteed to be.close()d once_run_animationis entered; a failure during scene-building leaks it until GC. A proper fix needs wrapping ~350 lines of scene construction in try/except — riskier than the bounded leak for a release, so left as a follow-up.pytest-randomly(order-dependent failures, unrelated to these changes) is worth a separate stabilization issue.Breaking change
🤖 Generated with Claude Code