Skip to content

Enh/acceleration earth to rocket frame #834

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

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from

Conversation

gdilbaz
Copy link

@gdilbaz gdilbaz commented Jun 23, 2025

Pull request type

  • Code changes (bugfix, features)

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 tests -m slow --runslow) have passed locally
  • CHANGELOG.md has been updated (if relevant)

Current behavior

Currently, there is no functionality in the code that performs the transformation of acceleration from the Earth frame to the Body frame.

New behavior

This PR adds acceleration tranformation from Earth frame to Body frame.

Breaking change

  • Yes
  • No

Additional information

I didn't add the feature as "axial_acceleration" as specified in the original issue in order to avoid confusion. Instead, I chose a clearer name "acceleration_earth_to_body_frame" which better represents what was originally in mind.

@gdilbaz gdilbaz requested a review from a team as a code owner June 23, 2025 10:23
Copy link
Member

Choose a reason for hiding this comment

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

please remove these .txt files

Copy link
Member

Choose a reason for hiding this comment

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

@Lucas-Prates , perhaps your latest PR introduced this problem of creating .txt when running Monte Carlo tests. Could you double check that please?

Copy link
Author

Choose a reason for hiding this comment

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

I'll look into that didn't realised thoese were included. Thanks for pointing it out!

@Gui-FernandesBR Gui-FernandesBR requested a review from Copilot June 23, 2025 12:19
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds a new property to transform acceleration from the Earth frame to the Body frame, improving the simulation of rocket dynamics. It also updates the README with a new badge and the changelog with an entry for the new feature.

  • Adds the acceleration_earth_to_body_frame property in the Flight class.
  • Updates README.md to include a new Ask DeepWiki badge.
  • Updates CHANGELOG.md with an entry for the new acceleration transformation feature.

Reviewed Changes

Copilot reviewed 3 out of 6 changed files in this pull request and generated no comments.

File Description
rocketpy/simulation/flight.py Adds a new cached property for acceleration transformation
README.md Introduces a new badge link
CHANGELOG.md Documents the addition, but with a naming inconsistency
Comments suppressed due to low confidence (1)

CHANGELOG.md:34

  • The changelog entry refers to 'axial_acceleration', but the Flight class now implements 'acceleration_earth_to_body_frame'. Please update the changelog to reflect the correct property name.
 - ENH: adds axial_acceleration attribute to the Flight class.- issue #529 [#834] (https://github.com/RocketPy-Team/RocketPy/pull/834)

Copy link
Member

Choose a reason for hiding this comment

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

In terms of coding, it is quite fine to me already.
I would just say that we need to implement unit tests for this method before merging.

@MateusStano, what do you think in terms of physics/dynamics? Anything to comment on?

@Gui-FernandesBR
Copy link
Member

I really love when people have the initiative to open up a PR to this repo. It makes our work so valuable and collaborative.

Thanks!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

4 participants