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: Refactor inertia calculations using parallel axis theorem #573

Merged
merged 5 commits into from
Mar 24, 2024

Conversation

Gui-FernandesBR
Copy link
Member

Pull request type

  • Code maintenance (refactoring, formatting, tests)

Checklist

  • Tests for the changes have been added (I don't think it's needed)
  • Docs have been reviewed and added / updated
  • Lint (black rocketpy/ tests/) has passed locally
  • All tests (pytest tests -m slow --runslow) have passed locally
  • CHANGELOG.md has been updated (if relevant)

Current behavior

We apply the parallel axis theorem in many parts of the code today.

New behavior

I created a parallel axis theorem function in the tools.py module, so now all the calculations are centralized in a single place.
This makes the code more readable, as it quickly guides the reader when understanding the code.

Don't repeat yourself (DRY).

Breaking change

  • No

Additional information

  • In the future we might wanna add acceptance/integration tests to verify if our inertia calculations (After summing different components) are aligned with expected physics phenomena.

@Gui-FernandesBR Gui-FernandesBR self-assigned this Mar 8, 2024
@Gui-FernandesBR Gui-FernandesBR requested a review from a team as a code owner March 8, 2024 16:09
Copy link

codecov bot commented Mar 8, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 72.70%. Comparing base (2a85ce8) to head (7b81e6e).
Report is 11 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop     #573      +/-   ##
===========================================
+ Coverage    72.66%   72.70%   +0.04%     
===========================================
  Files           59       59              
  Lines         9603     9618      +15     
===========================================
+ Hits          6978     6993      +15     
  Misses        2625     2625              

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

rocketpy/tools.py Outdated Show resolved Hide resolved
rocketpy/tools.py Outdated Show resolved Hide resolved
@phmbressan
Copy link
Collaborator

The review might seems pedantic because everything is working right now, but I find really important since this method might be used in the future in other parts of the code as a "black box" and will lead to incorrect results if not well documented or understood.

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.

Excellent, thank you for solving the review comments. Looks good.

@Gui-FernandesBR Gui-FernandesBR merged commit 0a4d89b into develop Mar 24, 2024
11 checks passed
@Gui-FernandesBR Gui-FernandesBR deleted the mnt/refactor-moment-of-inertia-conversions branch March 24, 2024 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

None yet

2 participants