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: Add _MotorPlots Inheritance to Motor Plots Classes #456

Merged
merged 2 commits into from
Nov 10, 2023

Conversation

MateusStano
Copy link
Member

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

Current behavior

A lot of repeated code in the motors' plots classes

New behavior

_SolidMotorPlots, _HybridMotorPlots and _LiquidMotorPlots now inherit _MotorPlots

Breaking change

  • Yes
  • No

Additional information

There are a lot of repeated code in _SolidMotorPlots and _HybridMotorPlots I could not find a good solution for it. I tried using multiple inheritance in _HybridMotorPlots but it went south really fast

Also, this is really useful for Motor Draws #436

@MateusStano MateusStano self-assigned this Nov 7, 2023
@MateusStano MateusStano added Motors Every propulsion related issue or PR Refactor Outputs Dedicated to visualizations enhancements like prints and plots labels Nov 8, 2023
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.

Removing 830 and adding 134 code lines, this is impressive. Good usage of parent-children classes arquitecture, loved that.

Regarding the "duplicated" code that you mentioned in the description. I don't think that is a huge problem. The methods are well documented so I don't see a big problem here, at least not for now.

We should do the same thing with the motor prints I guess.

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.

I am also very fond of this PR due to the code simplifications and architectural sense.

Just one comment that so that we pay attention to this mirroring of implementations:

  • The code architecture is repeated (there will be three identical inheritances between Motor, MotorPrints and MotorPlots);
  • I don't think this repetition is a good practice in the long term due to the fact that every change in the code hierarchy must be repeated twice (in prints and plots).

Nonetheless, the benefits of separating prints and plots currently outweighs these small issues by a good margin and this PR clearly simplifies the code. Well done.

@Gui-FernandesBR
Copy link
Member

I am also very fond of this PR due to the code simplifications and architectural sense.

Just one comment that so that we pay attention to this mirroring of implementations:

  • The code architecture is repeated (there will be three identical inheritances between Motor, MotorPrints and MotorPlots);
  • I don't think this repetition is a good practice in the long term due to the fact that every change in the code hierarchy must be repeated twice (in prints and plots).

Nonetheless, the benefits of separating prints and plots currently outweighs these small issues by a good margin and this PR clearly simplifies the code. Well done.

@phmbressan do you see another alternative for the problem you mentioned? Out of curiosity...

@Gui-FernandesBR Gui-FernandesBR merged commit 445a57d into develop Nov 10, 2023
9 checks passed
@Gui-FernandesBR Gui-FernandesBR deleted the enh/motorplots-inheritance branch November 10, 2023 13:22
@Gui-FernandesBR Gui-FernandesBR added this to the Release v1.X.0 milestone Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Motors Every propulsion related issue or PR Outputs Dedicated to visualizations enhancements like prints and plots Refactor
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

3 participants