RocketPy

RocketPy copied to clipboard

Pull request type

• [X] 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):

  • [X] Tests for the changes have been added
  • [X] Docs have been reviewed and added / updated if needed
  • [X] Lint (black rocketpy) has passed locally and any fixes were made
  • [x] 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:

• [X] Change on variable name of arguments from rocket and motor class (refer to #181 )
• [x] Distance vs position, a perspective change on the way we input values at rocket class (refer to #195)
• [X] Adding simple linear extrapolation model to describe the variation of center of mass position for the liquid+solid propellant phases
• [X] Adding example notebook considering hybridmotor (refer to #196)
• [x] Allows for reading .rse files for your motor

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

Does this introduce a breaking change?

• [X] 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.

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

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

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

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

@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).

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

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.

Task linked: CU-864dp0fng Hybrid Motors

Replaced by #233