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: Air Brakes #426

Merged
merged 81 commits into from
Feb 7, 2024
Merged

ENH: Air Brakes #426

merged 81 commits into from
Feb 7, 2024

Conversation

MateusStano
Copy link
Member

@MateusStano MateusStano commented Oct 4, 2023

Pull request type

  • Code changes (bugfix, features)
  • Code maintenance (refactoring, formatting, tests)
  • ReadMe, Docs and GitHub updates
  • Other (please describe):

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

New behavior

A simple implementation of an airbrakes simulation has been added. The added code:

  • Class Airbrakes: Inherits from AeroSurface. Defines an airbrakes system with a cd curve that is a function of the deployed level and mach number.
  • Class Controller: Defines a controller that has a controller_function. This function is defined by the user and can alter the state of any number of observed_objects. The controller will call the controller_function every sampling_rate (in simulation time).
  • add_controllers method in Rocket: Similar to add_surfaces
  • add_airbrakes method to Rocket: Similar to the aero surfaces add methods. Simplifies the addition of the air brakes for the user

Breaking change

  • Yes
  • No

MateusStano and others added 5 commits September 29, 2023 17:56
Co-authored-by: Pedro Henrique Marinho Bressan <[email protected]>
Co-authored-by: Giovani Hidalgo Ceotto <[email protected]>
Co-authored-by: Pedro Henrique Marinho Bressan <[email protected]>
Co-authored-by: Giovani Hidalgo Ceotto <[email protected]>
Co-authored-by: Pedro Henrique Marinho Bressan <[email protected]>
Co-authored-by: Giovani Hidalgo Ceotto <[email protected]>
@Gui-FernandesBR Gui-FernandesBR added this to the EuRoC 2023 milestone Oct 9, 2023
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@MateusStano MateusStano changed the title Enh/air brakes ENH: Air Brakes Oct 9, 2023
@MateusStano MateusStano marked this pull request as ready for review October 9, 2023 12:53
rocketpy/rocket/rocket.py Outdated Show resolved Hide resolved
@Gui-FernandesBR Gui-FernandesBR added the Enhancement New feature or request, including adjustments in current codes label Oct 9, 2023
Copy link
Member

@giovaniceotto giovaniceotto left a comment

Choose a reason for hiding this comment

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

This PR looks great. Simple yet powerful implementation of controllers. I was amazed by how such a simple controller could perfectly make the rocket reach the desired apogee.

@phmbressan @brunosorban and I helped in this PR at its infancy, but the example created by @MateusStano is fantastic. Hats off! 🚀

I do have one issue regarding the airbrakes.state_history attribute. I am not sure storing the state history in the airbrake instance is perfectly appropriate. I see a couple of problems:

  • airbrakes.state_history is empty before a Flight instance is created.
  • airbrakes.state_history may contain the history of multiple flights if various Flight instances are created using it, unless it is manually reset.
  • Plots related to airbrakes.state_history are totally empty before a flight.

That is why I wonder if this should be stored in the Flight class somehow, or if it is ok to have the airbrake instance mutate during flight. I would like a third opinion on this. @phmbressan @Gui-FernandesBR, any quick thoughts?

And a quick note regarding sources. I believe that the airbrake drag curve comes from a European Team. If confirmed, we should ask their permission to use the data and cite them.

*If fast approval is needed, we can go ahead and merge this PR and I'll create an issue to handle what is left afterwards. I hereby state that this PR is fully functional as far as I was able to review and test it. What is still necessary is:

  • Tests
  • Detailed docs
  • airbrakes.state_history discussion
  • Proper citation of data sources

@MateusStano
Copy link
Member Author

Getting back to this one:

  • c0408ce --> Added option to have the air brakes' cd curve substitute the rocket's power of drag curve. This is important for use cases with wind tunnel data, and maybe even certain CFD simulations

  • b11b21d --> Make the Controller class and all related attributes private

  • 11df71f --> This one requires some discussion. Because we are now saving all variables of interest in the controller observed_attributes, interacting with the controller class is pretty important. However, since we are making this class private, I tried finding a way around the need to interact with it. This was done with the get_controller_observed_variables cached property. Please pay special attention to this in your review

Final comment here is that we are definitely going to be missing the #273 Sensors implementation. I believe improving RocketPy's control capabilities should be our priority from here on out

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.

Nice changes since the previous reviews. I think we are quite close to conclude this one. I've made some minor comments that I think should contribute to the PR.

Tests could be better (i.e. faster) but I understand it doesn't need to be perfect right now.

The PR already allows users to simulate they airbrakes in RocketPy, which is remarkable!

rocketpy/rocket/rocket.py Show resolved Hide resolved
rocketpy/control/controller.py Outdated Show resolved Hide resolved
rocketpy/control/controller.py Show resolved Hide resolved
rocketpy/control/controller.py Show resolved Hide resolved
rocketpy/rocket/aero_surface.py Outdated Show resolved Hide resolved
rocketpy/simulation/flight.py Show resolved Hide resolved
docs/user/airbrakes.rst Outdated Show resolved Hide resolved
docs/user/airbrakes.rst Outdated Show resolved Hide resolved
rocketpy/rocket/aero_surface.py Show resolved Hide resolved
rocketpy/rocket/aero_surface.py Outdated Show resolved Hide resolved
rocketpy/rocket/aero_surface.py Outdated Show resolved Hide resolved
rocketpy/rocket/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.

Amazing pull request, these are truly some of the most interesting graphs I have seen in RocketPy.

Great to see more than 100 comments on this PR request with inputs and suggestion.

Superb work @MateusStano in putting this all together.

rocketpy/prints/aero_surface_prints.py Show resolved Hide resolved
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.

LGTM, @MateusStano whenever you feel confortable, please merge it.

@Gui-FernandesBR Gui-FernandesBR removed the request for review from brunosorban February 5, 2024 05:10
Copy link
Member

@giovaniceotto giovaniceotto left a comment

Choose a reason for hiding this comment

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

A masterpiece.

It is hard to make it 100% future proof, but this is definitely the best that could have been done for now. I am eager to see how control will evolve in RocketPy. Couldn't ask for a better start.

@MateusStano MateusStano merged commit 1132a11 into develop Feb 7, 2024
11 checks passed
@MateusStano MateusStano deleted the enh/air-brakes branch February 7, 2024 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement New feature or request, including adjustments in current codes
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

5 participants