Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

MNT: Encapsulate quaternion conversions #537

Merged
merged 3 commits into from
Jan 25, 2024

Conversation

Gui-FernandesBR
Copy link
Member

Pull request type

  • Code maintenance (refactoring, formatting, tests)

Checklist

  • Tests for the changes have been added (if needed)
  • Docs have been reviewed and added / updated
  • Lint (black rocketpy/ tests/) has passed locally
  • All tests (pytest --runslow) have passed locally
  • CHANGELOG.md has been updated (if relevant)

Current behavior

We take quaternions (euler parameters) and calculate euler angles inside the flight class, but this code cannot be reused in other applications.

New behavior

  • The quaternions -> angles conversions were moved to the tools.py module, thus allowing for other classes to access these functions.
  • The Flight class was adjusted to use the new functions.

Breaking change

  • No (I'm not sure if we should create a new .py file just to put these functions or not.)

Additional information

This is going to be quite useful for the rocketpy.simulation.FlightDataImporter class.

@Gui-FernandesBR Gui-FernandesBR added Refactor Flight Flight Class related features labels Jan 22, 2024
@Gui-FernandesBR Gui-FernandesBR added this to the Release v1.X.0 milestone Jan 22, 2024
@Gui-FernandesBR Gui-FernandesBR self-assigned this Jan 22, 2024
@Gui-FernandesBR Gui-FernandesBR requested a review from a team as a code owner January 22, 2024 20:57
Copy link

codecov bot commented Jan 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (08d3a42) 72.32% compared to head (b55fdd2) 72.34%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #537      +/-   ##
===========================================
+ Coverage    72.32%   72.34%   +0.01%     
===========================================
  Files           56       56              
  Lines         9389     9393       +4     
===========================================
+ Hits          6791     6795       +4     
  Misses        2598     2598              

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

Copy link
Collaborator

@phmbressan phmbressan left a comment

Choose a reason for hiding this comment

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

The documentation helps, but I have a minor issue with the naming schema of the created functions.

You certainly have more experience on quaternions than I, so I would like to hear your opinion:

  • Except the precession one, the names end up with the angle naming convention used for RocketPy (phi, theta).

  • I know this is a common angle convention, but I am not sure of it is universal. So I believe it would be better to name them after their "physical angle", such as precession and nutation.

@Gui-FernandesBR
Copy link
Member Author

The documentation helps, but I have a minor issue with the naming schema of the created functions.

You certainly have more experience on quaternions than I, so I would like to hear your opinion:

  • Except the precession one, the names end up with the angle naming convention used for RocketPy (phi, theta).
  • I know this is a common angle convention, but I am not sure of it is universal. So I believe it would be better to name them after their "physical angle", such as precession and nutation.

You have a good point.
I solved that with the cdb42ec.

Can you please re-review it?

@Gui-FernandesBR Gui-FernandesBR merged commit 821518a into develop Jan 25, 2024
11 checks passed
@Gui-FernandesBR Gui-FernandesBR deleted the mnt/flible_quaternions_convertions branch January 25, 2024 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Flight Flight Class related features Refactor
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

3 participants