ENH: RailButtons Class

[MateusStano]

Pull request type

Please check the type of change your PR introduces:

• Code base additions (bugfix, features)

Pull request checklist

• 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

Changes

• Created RailButtons Class In AeroSurfaces.py
• Changed Rocket.railButtons for Rocket.rail_buttons

The only question I have here is, the class I created receives in the init the upper_button_position and lower_button_position arguments. The setRailButtons method receives a position argument that is a list. Should we change this to the upper and lower button arguments?

The only advantage there is to using a list is that you don't need to specify an order, which can be more accessible in some cases given the rocket coordinate system.

[giovaniceotto]

When I started reviewing, I really liked the idea... but then I stumbled upon two issues:

• I don't see the relationship between rail buttons and the AeroSurfaces class. Rail buttons have no aerodynamic properties in RocketPy. Furthermore, the changes made to the AeroSurfaces class in Enh/change aero surfaces class #341 seem strange to me... it can be two very different things now, and it is not clear when which is which.
• The current implementation violates a key pillar of modularity in terms of rocket components. Just as we did with the new motor classes in ENH: Liquid Motors #233, we need to guarantee that a component, be it a motor, a nose cone or a rail button, will work the same irrespective of the rocket that we add it to. This is the reason that we had to create a motor coordinate system, to guarantee that a motor can be completely defined without making any assumptions about the rocket that it will be used in. Then, in the addMotor method, we specify the missing link that allows for the conversion of all information needed to the rocket system. Notice how the NoseCone class has no position argument... it is specified only in the addNoseCone method. But in the case of rail buttons, its proposed class pretty much has 3 position arguments. It just doesn't seem like a promising idea to implement it as a separate entity like this, but I would love to be proven wrong.

To conclude, this PR adds 77 lines of code, while only removing 32. It seems it uses more lines of code to accomplish the same thing that was already implemented.

[MateusStano]

• I don't see the relationship between rail buttons and the AeroSurfaces class. Rail buttons have no aerodynamic properties in RocketPy. Furthermore, the changes made to the AeroSurfaces class in Enh/change aero surfaces class #341 seem strange to me... it can be two very different things now, and it is not clear when which is which.

Okay, so the reason this change was made was to fit the new Dispersion Class. The way we handle aerodynamic surfaces in the monte carlo simulation is:

• Create the AeroSurface object
• Use this object to create the pydantic DataClass object that also holds the standard deviation
• Add the DataClass object to the Dispersion Rocket

You can check this process in this notebook: https://github.com/RocketPy-Team/RocketPy/blob/6142519d26f8fccb70e0a1cea018395367a69709/docs/notebooks/dispersion_analysis/dispersion_class_usage.ipynb
It is a bit messy still but I think it is good enough to understand the process.

The old RailButton implementation was really hard to deal with in the dispersion class because it completely broke the pattern that worked for all the aerosurfaces. The solution was then to just make a RailButtons class, but to keep the implementation in the Rocket class exactly the same as it was with the named tuple.

Writing the RailButton class inside AeroSurfaces.py was just intended as a "future proof" implementation. When we eventually add drag calculations, the buttons will need to be treated as an Aerodynamic surface (or at least that is the idea for now). Currently the RailButton class and the AeroSurface class really have no relation. Also the intended way to add railbutton to the rocket is just the same as before, you are not meant to create a Rail Button object, just to retrieve it from the setRailButtons method

[Gui-FernandesBR]

I couldn't read all the thread yet, but I already can say that I agree with creating a class to Rail Buttons.
Currently they are a named tuple, which does not follow any other example in the code.
If parachutes, noses, tails, and fins are saved inside classes, the buttons should also be.
An extra benefits is that it facilitates quite a lot when dealing with the dispersion class.
If someday we decide to compute Cd of each individual part, having these little guys isolated in the could may also help us.

I agree when Giovani complains about being rocket independent. In that case, I think the buttons probably should be treated in the same way as the motors (using 1 cys to the object and other for the rocket).

[phmbressan]

I agree with the change of RailButtons from a namedtuple to its own class, since it gives more flexibility. The main issue that may be reconsidered to increase code parallelism is making the coordinate system of the rail buttons rocket independent.

There is a more general issue around the documentation of the position parameter in the AeroSurfaces class and its components, but this might be discussion for another PR.

[MateusStano]

Hey, with the new AeroSurface done, I adapted the RailButtons class and all tests are passing but two questions remain:

• In the class documentation it is mentioned that the upper_rail_position and lower_rail_position use the same coordinate system as the Rocket that the buttons will be a part of. This is different from what we did with the aerosurfaces and motors. However, RailButtons are different because they are not "added" to the rocket, but "set". There is no method to set the rail buttons given just a RailButton object. Should this be different?

• The RailButtons class does not inherit the AeroSuface class like it was planned. This is done because all the @abstracmethod's would have to be redefined to return nothing since they do not generate any lift. When drag is added, the inheritance should be added though. What could be done is implement geometrical parameters to the RailButtons class __init__ and change the setRailButtons method of the Rocket class. This would avoid future breaking changes. Is it worth it to do this now?

The first topic would be an easy change. I would change the __init__ of the RailButtons to receive the distance between the buttons, and the setRailButtons method would receive the RailButtons object along with the position of the lower (or upper) button.

The second topic not so much....

[Gui-FernandesBR]

Hey, with the new AeroSurface done, I adapted the RailButtons class and all tests are passing but two questions remain:

• In the class documentation it is mentioned that the upper_rail_position and lower_rail_position use the same coordinate system as the Rocket that the buttons will be a part of. This is different from what we did with the aerosurfaces and motors. However, RailButtons are different because they are not "added" to the rocket, but "set". There is no method to set the rail buttons given just a RailButton object. Should this be different?

I think it is worth it to have rail buttons being defined and treated the same way we are already doing with the other components. My suggestions is to just ask the distance_between_buttons as an argument of rail buttons class. This would allow the user to add this buttons to any rocket later. Is that functionality something really useful? Well, perhaps not, but at least we keep consistency with the other components.

In the Rocket class, the "position" that is requested when adding a new component would make the same job in the buttons case: you just ask what is the position of one button (for example, the lowest button in the rocket) and then calculate the position of the second one.

• The RailButtons class does not inherit the AeroSuface class like it was planned. This is done because all the @abstracmethod's would have to be redefined to return nothing since they do not generate any lift. When drag is added, the inheritance should be added though. What could be done is implement geometrical parameters to the RailButtons class __init__ and change the setRailButtons method of the Rocket class. This would avoid future breaking changes. Is it worth it to do this now?

What do expect to add as "geometrical parameters"? In my opinion, we would have to require only the Cd and the frontal area of the rail button if we wanted to calculate its drag force. In the very particular case where the user do not know the Cd of the rail button, we could either assume a pre-defined Cd or estimate its values based on geometric parameters.

But I have another question too:

• Should we really proceed with the "rail button" name? Launch lugs are also an option when designing a rocket, and perhaps we could bring a more flexible name here. I'm confortable with rail buttons if we don't have another suggestion tho, but try to think about that please.

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[MateusStano]

All done! Ready for a quick re-review @Gui-FernandesBR @phmbressan

[phmbressan]

It might be a commentary regarding __repr__ more generally for the AeroSurfaces PR. However, why to remove the __repr__ (and __str__)? I understand that there is already plenty of info in allInfo, however the __repr__ is quite important to make sense of the object in a compact way, for instance mentioning the self.name attribute.

[MateusStano]

Because of this:

If you have any suggestions for a __repr__ that does not look weird in the component_tuple I'd love to hear it

[Gui-FernandesBR]

I hereby confirm that tests are working on my computer,

@MateusStano , I just left a few comments to you regarding docstrings. That should be very easy to solve tho.
You have my approvement after fixing such small details.

Great one!