Skip to content

Conversation

Copy link

Copilot AI commented Dec 3, 2025

Pull request type

  • Code maintenance (refactoring, formatting, tests)

Checklist

  • All tests (pytest tests -m slow --runslow) have passed locally

Current behavior

The quaternion derivative calculation in u_dot_generalized_3dof had identical code blocks duplicated in two places (lines 1958-1962 and 1989-1993):

# Block 1: Normal misalignment case
e0_dot = 0.5 * (-omega1_wc * e1 - omega2_wc * e2 - omega3_wc * e3)
e1_dot = 0.5 * (omega1_wc * e0 + omega3_wc * e2 - omega2_wc * e3)
# ... same formulas

# Block 2: Anti-aligned case (duplicated)
e0_dot = 0.5 * (-omega1_wc * e1 - omega2_wc * e2 - omega3_wc * e3)
e1_dot = 0.5 * (omega1_wc * e0 + omega3_wc * e2 - omega2_wc * e3)
# ... same formulas again

New behavior

Refactored to compute omega_body first in all branches, then calculate quaternion derivatives once:

  • First: Determine omega_body based on alignment state (normal, anti-aligned, or aligned/None)
  • Then: Compute quaternion derivatives in a single block if omega_body is not None

This reduces maintenance burden—formula updates only need to happen in one place.

Breaking change

  • No

Additional information

Addresses code review feedback on PR #883.


💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.

Copilot AI and others added 2 commits December 3, 2025 21:39
…eneralized_3dof

Co-authored-by: aZira371 <99824864+aZira371@users.noreply.github.com>
Co-authored-by: aZira371 <99824864+aZira371@users.noreply.github.com>
Copilot AI changed the title [WIP] Address feedback on 3-dof lateral motion improvement Refactor: eliminate quaternion derivative code duplication in u_dot_generalized_3dof Dec 3, 2025
Copilot AI requested a review from aZira371 December 3, 2025 21:43
@aZira371 aZira371 marked this pull request as ready for review December 3, 2025 21:53
@aZira371 aZira371 requested a review from a team as a code owner December 3, 2025 21:53
@codecov
Copy link

codecov bot commented Dec 3, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 80.39%. Comparing base (b1eb6cb) to head (c92d5f0).
⚠️ Report is 1 commits behind head on enh/3-dof-lateral-motion-improvement.

Additional details and impacted files
@@                           Coverage Diff                            @@
##           enh/3-dof-lateral-motion-improvement     #903      +/-   ##
========================================================================
- Coverage                                 80.91%   80.39%   -0.52%     
========================================================================
  Files                                       107      106       -1     
  Lines                                     13486    13043     -443     
========================================================================
- Hits                                      10912    10486     -426     
+ Misses                                     2574     2557      -17     

☔ View full report in Codecov by Sentry.
📢 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.

Copy link
Member

@Gui-FernandesBR Gui-FernandesBR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could separate the whole function into smaller methods... It makes code more readable and prevents refactors like this.

@aZira371 aZira371 merged commit 70c6b5f into enh/3-dof-lateral-motion-improvement Dec 3, 2025
25 of 26 checks passed
@aZira371 aZira371 deleted the copilot/sub-pr-883 branch December 3, 2025 22:04
@aZira371
Copy link
Collaborator

aZira371 commented Dec 3, 2025

Could separate the whole function into smaller methods... It makes code more readable and prevents refactors like this.

@Gui-FernandesBR separating the whole u_generalized_3dof function ? maybe yes that can be done like can create a separate helper function which does the whole wc thing. I saw your comment after merging. Will add a TODO for it, can take this up later. But will keep the advice in mind.

Gui-FernandesBR pushed a commit that referenced this pull request Dec 3, 2025
* ENH: addition of bella lui based 3 dof and 6 dof comparison notebook

- ENH: a new notebook bella_lui_3dof_vs_6dof.ipynb which uses new implementations of weathercocking model on 3dof

* ENH: addition of weathercocking model to flight.py

- ENH: introduced new weathercock_coeff parameter in Flight.init (default: 1.0)

- ENH: updated u_dot_generalized_3dof to compute quaternion derivatives proportional to misalignment with relative wind

- ENH: angular velocity = weathercock_coeff * sin(misalignment_angle)

* ENH: added tests for weathercocking to test_flight_3dof.py

- ENH: unit tests added for weathercocking to check whether weathercock_coeff=0 results in fixed attitude (no quaternion change).

- MNT: format and lint updates for new additions

* DOC: updating 3 dof documentation and corresponding index.rst

- DOC: added 3 dof and 6 dof comparison analysis to three_dof_simulation.rst

- DOC: updated iusers/index.rst to build three_dof_simulation.rst

- MNT: deleted the bella_lui_3dof_vs_6dof_comparison.ipynb as the doc now covers this section

* MNT: corrections to test_flight_3dof.py

- MNT: corrected doc string to represent correct orientation

- MNT: improved tolerance of check on quaternion derivative which should be ideally very small when axes are aligned

* MNT: docstring corrections to flight.py around new weathercocking implementation

* BUG: correction of singularity bug during align on vectors in weathercocking

- BUG: implemented a dot product check to ensure that singularity bug is avoided when rocket body and wind velocity are anti aligned to make rocket statically stable (in u_dot_generalized_3dof)

- MNT: removed redundant double assignment of e0 and w0 vectors within u_dot_generalized_3dof

- MNT: format correction to test_flight_3dof.py

* MNT: updating location of 3 dof doc in users index.rst

- MNT: 3 dof documentation only referred in users section/getting started

- MNT: correction of docstring in flight.py

- MNT: corrected unit_vector call when defining rotation axis in u_dot_generalized_3dof

- MNT:  docstring correction in test_flight_3dof.py

- ENH: test coverage added for anti alignment case in weathercock model to test_flight_3dof.py

* DOC: three_dof_simulation.rst update to add explanation of weather cocking coeff usage and value.

* MNT: changed default value of weather_coeff in flight.py and added fixtures to test_flight_3dof.py

* MNT: shifting test_flight_dof.py to integration tests.

- MNT: fixed default weathercock_coeff value in flight.py to match 3 dof behaviour by default

- MNT: corrected fixtures and docstrings in test_flight_3dof.py

* MNT: docsrting update in test_flight_3dof.py

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* MNT: Update of docstring in test_flight_3dof.py

- MNT: correction of docstring now that fixtures are used.

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* DOC: Update of three_dof_simulation.rst

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Update tests/integration/simulation/test_flight_3dof.py

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Docstring Update tests/integration/simulation/test_flight_3dof.py

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* docstring Update tests/integration/simulation/test_flight_3dof.py

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* docstring Update docs/user/three_dof_simulation.rst

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* Docstring Update rocketpy/simulation/flight.py

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

* MNT: Docstring updates to various files related to 3 dof

* MNT: unit vector edge case check in flight.py

- MNT: perp_axis singularity value error implemented to handle edge case for perp_axis being parallel to body axis in weathercocking model of 3 dof.

* DOC: Add note about motor file paths in 3-DOF comparison section (#902)

* Initial plan

* DOC: Add note about motor file paths in 3-DOF comparison section

Co-authored-by: aZira371 <99824864+aZira371@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: aZira371 <99824864+aZira371@users.noreply.github.com>

* MNT: eliminate quaternion derivative code duplication in u_dot_generalized_3dof (#903)

* Initial plan

* Refactor: eliminate quaternion derivative code duplication in u_dot_generalized_3dof

Co-authored-by: aZira371 <99824864+aZira371@users.noreply.github.com>

* Improve comment clarity in weathercocking aligned case

Co-authored-by: aZira371 <99824864+aZira371@users.noreply.github.com>

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: aZira371 <99824864+aZira371@users.noreply.github.com>

* DOC: CHANGELOG.md update to reflect implementation of 3 dof lateral motion improvement

---------

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <198982749+Copilot@users.noreply.github.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.

3 participants