BUG: adding rocket radius parameter to rail buttons class

juliomachad0 commented 4 months ago

Pull request type

[X] Code changes (bugfix, features)

Checklist

[X] Lint (black rocketpy/ tests/) has passed locally
[X] All tests (pytest tests -m slow --runslow) have passed locally

Current behavior

Issue 606 resolution. The RailButtons class did not have the rocket_radius parameter, which led to some errors when the user used the class in the same way as other aerosurfaces. The set_rail_buttons method had to be used. This is not technically an error, but I see it as a difficulty in the sense that there is a class that deviates from the usage pattern of other classes.

New behavior

I updated the RailButtons class by adding the rocket_radius parameter, to avoid the simulation failing if the user uses the class as they used the others. I also modified some tests to consider the new parameter.

Breaking change

[X] No

codecov[bot] commented 4 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 73.90%. Comparing base (44cd770) to head (429d59d). Report is 1 commits behind head on develop.

Additional details and impacted files

```diff @@ Coverage Diff @@ ## develop #637 +/- ## =========================================== + Coverage 73.85% 73.90% +0.05% =========================================== Files 70 70 Lines 10035 10036 +1 =========================================== + Hits 7411 7417 +6 + Misses 2624 2619 -5 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

Gui-FernandesBR commented 4 months ago

First of all, congratulations @julio for creating this PRs. It's is really nice that you took the initiative of solving the issue on your own and propose your solution here. As described here https://github.com/RocketPy-Team/RocketPy/issues/561#issue-2149809620, your approach is not the only one that could be used to address the problem.

Unfortunately, this PR violates a basic principle of breaking change and therefore it's at least complicated doe us to merge and release the way it is. I think I should explain in why.

Imagine the case an user, running rocketpy v1.4.0 (latest), decides to define a code like this:

rail_button_object = RailButtons(
    10, # buttons_distance
    45, # angular_position
    "Rail Buttons", # name
)

In your implementation, however, this very same code would mean...

rail_button_object = RailButtons(
    10, # rocket_radius
    45, # buttons_distance
    "Rail Buttons", # angular_position
    # please notice the code would assume name = "Rail Buttons", but also angular_position = "Rail Buttons"  
)

Therefore, if we accept your implementation, upgrading rocketpy from v1.4.0 to v.1.5.0 would cause make the previous code to stop running correctly, which may be a serious problem (or at least something that we should avoid).

Also, the fact that you had to modify tests when opening this PR already triggered me in this concern.

My suggestions:

1 - If you really want to add the rocket_radius argument to the RailButtons class, it should be added as an optional argument by the end of the arguments list. For example:

    def __init__(
        self,
        buttons_distance,
        angular_position=45,
        name="Rail Buttons",
        rocket_radius=None, # initializes with None if user does not set it.
    ):

2 - I think we should discuss this implementation before you continue your progress. My question is: Should we really add rocket_radius to the init of the RailButtons class (and maybe also to the airbrakes), or should we modify the u_dot_generalized to deal with surfaces when they have the ref_factor attribute assigned and not. Obs.: I really like simple solutions and I hate that PRs stay open for so long, but I would dislike it even more to know that we didn`t even consider alternative solutions.

Obs.: Please remember that - ideally - each aerosurface is rocket-independent.

RocketPy-Team / RocketPy