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

ENH: Adding Stability Margin with Mach dependency #377

Merged
merged 19 commits into from
Oct 6, 2023

Conversation

Gui-FernandesBR
Copy link
Member

Pull request type

Please check the type of change your PR introduces:

  • Code base additions (bugfix, features)

Pull request checklist

Please check if your PR fulfills the following requirements, depending on the type of PR:

  • Code base additions (for bug fixes / features):

    • Tests for the changes have been added
    • Docs have been reviewed and added / updated if needed
    • Lint (black rocketpy) has passed locally and any fixes were made
    • All tests (pytest --runslow) have passed locally

What is the current behavior?

  • The flight.staticMargin doesn't seem good enough nowadays. It do not consider the variation of the center of pressure position as the mach number varies during the flight.
  • Users usually have troubles to compare stability across different rocket simulators.

What is the new behavior?

  • Implementation followed the instructions/requirements established here: Stability Margin with Compressibility Factor #350
  • The Rocket and Flight classes now communicates very well to compute more robust stabilityMargin attribute, while the trajectory simulation is not being affected.
  • Everything thanks to the most magical class of rocketpy: Function

Does this introduce a breaking change?

  • Yes (static margin is no longer a synonym of stability margin in rocketpy).

Other information

  • I tried to do a chart like this, but I got no success at all. This can be done in the future.
  • Please, review the changes first, and then we update the notebooks.

@Gui-FernandesBR Gui-FernandesBR added Enhancement New feature or request, including adjustments in current codes Aerodynamics Any problem to be worked on top of RocketPy's Aerodynamic Flight Flight Class related features labels Jun 11, 2023
@Gui-FernandesBR Gui-FernandesBR added this to the EuRoC 2023 milestone Jun 11, 2023
@Gui-FernandesBR Gui-FernandesBR self-assigned this Jun 11, 2023
@Gui-FernandesBR Gui-FernandesBR linked an issue Jun 11, 2023 that may be closed by this pull request
rocketpy/Rocket.py Outdated Show resolved Hide resolved
rocketpy/Rocket.py Outdated Show resolved Hide resolved
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.

Overall, the Mach dependency is a great addition.

This PR still needs to be updated to follow snake_case conventions before approval.

rocketpy/Rocket.py Outdated Show resolved Hide resolved
rocketpy/plots/rocket_plots.py Outdated Show resolved Hide resolved
@Gui-FernandesBR Gui-FernandesBR marked this pull request as draft September 5, 2023 12:29
@Gui-FernandesBR
Copy link
Member Author

I'm waiting for the #395 PR to be merged before solving the conflicts here.

@MateusStano MateusStano changed the base branch from beta/v1.0.0 to develop September 29, 2023 22:57
@MateusStano MateusStano marked this pull request as ready for review September 29, 2023 22:59
@MateusStano
Copy link
Member

Hey! I changed a few things, mostly structure wise.

  • I re-added static margin because it is still useful (it is just a special case of the stability margin)
  • I made stability margin a 2D Function, so now it makes a 3D plot in Rocket.all_info()
  • Flight calculates the stability margin through the flight and plots it

Think this is ready for a review

@Gui-FernandesBR
Copy link
Member Author

Gui-FernandesBR commented Sep 30, 2023

Good work, @MateusStano , thank you so much for improving the stability margin calculation in RocketPy, this was a huge gap since the beginning.

I made some small adjusts to improve plots and prints. You can check them by my last 2 commits.
Some tests that I think you would like to see:
image

image

image

I want to approve this PR by I was the one who created.
Well, please consider this as resolved/approved, and feel free to merge into develop, there're no breaking changes.

rocketpy/simulation/flight.py Outdated Show resolved Hide resolved
rocketpy/rocket/rocket.py Show resolved Hide resolved
Comment on lines +512 to +515
self.static_margin.set_source(
lambda time: (self.center_of_mass(time) - self.cp_position(0))
/ (2 * self.radius)
* self._csys
Copy link
Collaborator

Choose a reason for hiding this comment

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

I have no problems with this, but out of curiosity why define a lambda source and discretize right after?

Wouldn't it work using Function operations directly? (same comment to the stability_margin evaluation)

Copy link
Member

Choose a reason for hiding this comment

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

For some reason, static_margin used to break pretty badly with some liquid and hybrid motors. We added this discretization when developing the liquid and hybrid motors, but it might have been done before we started discretizing things in those classes

I just tested it and it seems it can be remover without issues

@MateusStano MateusStano merged commit 43fdd8a into develop Oct 6, 2023
9 checks passed
@MateusStano MateusStano deleted the enh/new-stability-margin branch October 6, 2023 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Aerodynamics Any problem to be worked on top of RocketPy's Aerodynamic Enhancement New feature or request, including adjustments in current codes Flight Flight Class related features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Stability Margin with Compressibility Factor
4 participants