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: Support for Hybrid Motors part 1 #198

Closed
wants to merge 66 commits into from

Conversation

Gui-FernandesBR
Copy link
Member

@Gui-FernandesBR Gui-FernandesBR commented Jun 22, 2022

Pull request type

  • 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?

RocketPy currerntly does not support Hybrid motor, which is such a limitation for our project right now. Therefore @ompro07 and @MrGribel commited theirselves to work on the different approaches to implement the non-soldid motors. However, the roadtrip seems to be more difficult than what we expected, and right now we are pulling simple features that will allow further enhancements.

What is the new behavior?

In this PR we submitting the following aditions:

Also, we adjusted all tests files to accomplish with those changes!! Hope it facilitates the final review.

Does this introduce a breaking change?

  • Yes
  • No

Other information

This is a 1st part of 2 different developments, The next steps will be started on next months and should include at least the following developments:

  • Rebuild equations of motion to accomplish with variable distance (rocket - propellant) [This will definetely causa a breaking change on our code]
  • Add more sophisticated models for variating center of mass position of propellant
  • Implement YAML files integration
  • Find a fancy way of receinving .csv files as input for motor attributtes such as mass, inertia and CM position with respect of time.

ompro07 and others added 30 commits June 2, 2022 16:42
The reference point for the calculation of the center of mass of the motor was altered from the geometric center of the propellant grain to the nozzle
distanceNozzlePropellant was changed for distanceNozzleMotorReference.
The variables were adjusted in the notebooks.
ENH: change of variables between rocket and motor
A method was added that allows calculating the variation of the center of mass of the hybrid motor using a linear interpolation.
@MateusStano
Copy link
Member

Heyy! Think I maneged to solve all he conflicts and fix all the tests!!

Only thing missing here is a test for a thrust source given by a .rse file. @ompro07 could you create a simple test using a .rse file?

Besides that I think this one is done. Will only be missing a classic @giovaniceotto review

@Gui-FernandesBR Gui-FernandesBR linked an issue Sep 9, 2022 that may be closed by this pull request
@Gui-FernandesBR Gui-FernandesBR changed the title WIP: Support for Hybrid Motors part 1 Support for Hybrid Motors part 1 Sep 10, 2022
@giovaniceotto
Copy link
Member

I am currently working on this review. It is quite a long PR and, unfortunately, it's been a while since I first read the code. It should take a couple of days for me to finish. My intention is to finish by, at most, Friday.

@Gui-FernandesBR
Copy link
Member Author

I am currently working on this review. It is quite a long PR and, unfortunately, it's been a while since I first read the code. It should take a couple of days for me to finish. My intention is to finish by, at most, Friday.

I recommend you to read the other pull requestd we have made untill to get at this point. Indeed it's a large PR, but if you take it step by step you're going to see there's no Rocket science here. The most difficult part I think is related to distances vs positions

@Gui-FernandesBR Gui-FernandesBR added Motors Every propulsion related issue or PR and removed feature labels Sep 14, 2022
@Gui-FernandesBR
Copy link
Member Author

@PatrickSampaioUSP is also set as reviewer to test if the code is working! We need to proceed with this one too.

@Gui-FernandesBR Gui-FernandesBR changed the title Support for Hybrid Motors part 1 ENH: Support for Hybrid Motors part 1 Sep 17, 2022
@PatrickSampaioUSP
Copy link
Contributor

@Gui-FernandesBR what about those conflicts? Usually you have to resolve the conflicts before the code review (merge the master branch into this branch, resolve the conflicts, then commit to this branch).

@giovaniceotto
Copy link
Member

Thanks @MateusStano for solving the merge conflicts. I'll continue my review.

@PatrickSampaioUSP
Copy link
Contributor

I read the changes, and I understood that those are some features that we want to publish in order to have a functional Motor class for some basic features of the Hybrid. However there are some aspects of the implementation that worries me:

  1. Almost all of the SolidMotor function are replicated in the HybridMotor, this confused me when I was looking the code, because I though that it was already the implementation for the Hybrid model, at the first time I would suggest to move the common functions to the Motor class, that way we would have only the HybridMotor functions on the class, thus being easier to understood what this class really does. However this will implicate in removing those function from the Motor class in the feature. Even though this code replication would be temporarily, this still worries me, because we don't know for how much time it would be like this, therefore I would prefer to implement the common functions on the Motor class or the HybridMotor class to inherit then from SolidMotor(and to explain that this is a temporary thing).
  2. I suggest the separation of files, the Motor file have already 13xx lines, so it's starting to become difficult to navigate on this file.
  3. A final suggestion would be to leave a comment explaining what we have done, we are publishing a minimal hybrid motor class in order to deliver a couple of features, however most of the fuctions are not implemented using a hybrid model, because at first glance it sounds like the hybrid models are already implemented.

@Gui-FernandesBR Gui-FernandesBR changed the base branch from develop to beta/v1.0.0 October 6, 2022 01:52
@MateusStano MateusStano added this to the Release v1.0.0 milestone Oct 30, 2022
@Gui-FernandesBR
Copy link
Member Author

Task linked: CU-864dp0fng Hybrid Motors

@giovaniceotto
Copy link
Member

giovaniceotto commented Mar 22, 2023

Replaced by #233

@Gui-FernandesBR Gui-FernandesBR deleted the hybrid-integration-v2 branch July 5, 2023 10:33
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 Motors Every propulsion related issue or PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Distances vs. Positions...
7 participants