Skip to content

MNT: store ref_factor on rocket aero surface components (#561) - #1129

Merged
Gui-FernandesBR merged 3 commits into
RocketPy-Team:developfrom
thatrandomasiandev:mnt/561-ref-factor-components
Sep 9, 2026
Merged

MNT: store ref_factor on rocket aero surface components (#561)#1129
Gui-FernandesBR merged 3 commits into
RocketPy-Team:developfrom
thatrandomasiandev:mnt/561-ref-factor-components

Conversation

@thatrandomasiandev

Copy link
Copy Markdown

Summary

  • Extend Components.component_tuple with ref_factor (default 1.0) and pass it through Components.add, serialization, and call sites that unpacked (component, position).
  • Compute and store ref_factor = (surface.rocket_radius / rocket.radius)**2 when adding aero surfaces; use the stored value in evaluate_center_of_pressure and rocket prints.
  • Add unit tests for Components storage/round-trip and the Rocket add path.

Fixes #561

Test plan

  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 python -m pytest tests/unit/rocket/test_components.py tests/unit/rocket/test_rocket.py -q
  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 python -m pytest tests/unit/rocket/ tests/unit/test_rail_buttons_bending_moments.py tests/integration/test_sensor.py tests/unit/simulation/test_flight.py -q

@thatrandomasiandev
thatrandomasiandev requested a review from a team as a code owner August 11, 2026 01:38
@Gui-FernandesBR
Gui-FernandesBR force-pushed the mnt/561-ref-factor-components branch from ab19b9a to f881d7e Compare August 14, 2026 00:16
@Gui-FernandesBR
Gui-FernandesBR force-pushed the mnt/561-ref-factor-components branch from f881d7e to ebe5b8b Compare August 15, 2026 12:16
@Gui-FernandesBR

Copy link
Copy Markdown
Member

This one needs a different approach rather than a rebase. Two compatibility problems, and the first is what is failing CI:

1. It breaks .rpy files that already exist.

FAILED tests/unit/test_utilities.py::test_load_from_rpy
ValueError: not enough values to unpack (expected 3, got 2)   (rocket.py:2345)

Rocket.from_dict destructures three elements out of data["aerodynamic_surfaces"], but every file serialized before this PR stores two. The namedtuple's defaults=(1.0,) covers Components.add, it does not cover destructuring of already-stored data.

2. It breaks the public API. for surface, position in rocket.aerodynamic_surfaces: is one of the most common idioms in user notebooks and it now raises ValueError. The PR description marks this as not a breaking change; it is one.

A backward-compatible shape would be to keep the 2-tuple iteration contract intact and expose ref_factor some other way (an attribute on the component, or a separate lookup), and to make from_dict accept both the 2- and 3-element forms. I would like to see test_load_from_rpy and the 2-tuple idiom both still working before this goes in.

3. Lint is one ruff format away — rocketpy/rocket/components.py, the self._components.append(...) call now fits on one line:

self._components.append(self.component_tuple(component, position, ref_factor))

Gui-FernandesBR and others added 2 commits September 9, 2026 11:08
Rocket.from_dict unpacked three fields from every serialized entry, so a
.rpy file written before ref_factor was stored failed to load with "not
enough values to unpack (expected 3, got 2)" -- which is what the committed
fixture tests/fixtures/utilities/flight_calisto_robust.rpy is, and what
every file a user already has saved is too. Regenerating the fixture would
have turned the suite green while leaving those files unreadable, so the
three loops take a trailing star instead and read either length. Nothing is
lost by ignoring the stored factor: add_surfaces derives it again from the
surface's own radius.

Two tests that reached develop after this branch opened, in RocketPy-Team#1169 and
RocketPy-Team#1170, iterate Components expecting pairs, and now get triples. Both take
the star as well, so a later field does not break them again.

Also fixes what CI would have failed on regardless of the above: pylint
C0415 for the two rocketpy imports inside test functions, now at the top of
the module, and one ruff formatting difference in components.py.

Simulation results are unchanged -- the 18 acceptance tests pass, and the
factor is the same number as before, computed once when the surface is
added rather than at every lift evaluation.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@codecov

codecov Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 91.24%. Comparing base (8f3a9ce) to head (2d1efe7).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #1129      +/-   ##
===========================================
+ Coverage    91.21%   91.24%   +0.02%     
===========================================
  Files          131      131              
  Lines        17618    17619       +1     
===========================================
+ Hits         16070    16076       +6     
+ Misses        1548     1543       -5     

☔ 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.

@Gui-FernandesBR
Gui-FernandesBR merged commit bc3fe73 into RocketPy-Team:develop Sep 9, 2026
9 checks passed
Gui-FernandesBR pushed a commit that referenced this pull request Sep 9, 2026
…ltistage

#1129 landed on develop while this branch was open and made Components
yield (component, position, ref_factor). This branch reads that collection
in five places, so 29 of its tests failed with "too many values to unpack
(expected 2)": axial_extent, two surface comprehensions and one draw loop
in multistage, plus a dict(composed.aerodynamic_surfaces) in the tests,
which failed differently again ("dictionary update sequence element #0 has
length 3"). All five take a trailing star now, so a further field does not
break them a third time.

Addressing the review, in the same order the comments were left:

- multistage.py was 810 lines holding four classes and a helper, and is now
  a package of one class per file, following the aero_surface layout that
  is already in rocketpy/rocket/. __init__ re-exports all five names, so
  every existing import path keeps working unchanged -- including the
  notebook's and the docstring cross-reference to axial_extent.
- multistage_mission_test_report.ipynb is removed. It was the author's own
  working file, as they confirmed, and nothing referenced it.

That leaves the two CI failures: ruff format over the notebook, multistage
and its tests, and the build-docs warning that multistage_mission.ipynb sat
in no toctree, which is now listed under Special Case Simulations next to
the sensors notebook. Removing the report notebook takes care of the second
warning of the two, since it was unreferenced as well.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

MNT: save ref_factor value when adding aero surfaces to a Rocket

2 participants