Skip to content

Conversation

@ompro07
Copy link
Contributor

@ompro07 ompro07 commented Jun 20, 2022

A method was added that allows calculating the variation of the center of mass of the hybrid motor using a linear interpolation.

Pull request type

Please check the type of change your PR introduces:

  • Code base additions (bugfix, features)
  • Code maintenance (refactoring, formatting, renaming, tests)
  • ReadMe, Docs and GitHub maintenance
  • Other (please describe):

Pull request checklist

Please check if your PR fulfills the following requirements, depending on the type of PR:

  • ReadMe, Docs and GitHub maintenance:

    • Spelling has been verified
    • Code docs are working correctly
  • Code base maintenance (refactoring, formatting, renaming):

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

Enter text here...

What is the new behavior?

Enter text here...

Does this introduce a breaking change?

  • Yes
  • No

Other information

Enter text here...
A method was added that allows calculating the variation of the center of mass of the hybrid motor using linear interpolation.

A method was added that allows calculating the variation of the center of mass of the hybrid motor using a linear interpolation.
@ompro07 ompro07 requested a review from giovaniceotto as a code owner June 20, 2022 18:54
@Gui-FernandesBR Gui-FernandesBR added Enhancement New feature or request, including adjustments in current codes NewFeature labels Jun 20, 2022
@Gui-FernandesBR
Copy link
Member

@ompro07 can you add a simple example of your feature working ?

@Gui-FernandesBR
Copy link
Member

Hey @ompro07 nice PR from your side. I've pointed 3 major comments that I think we can discuss in more details in order to improve this implementation and the next ones

1 . I definetely suggest we include an example of hybrid motor usage, at least for facilitating our verifications on future pull requests. I am suggesting the following example:

hybrid = HybridMotor(thrustSource=2000,   # These are dummy numbers, please add what makes more sense in each field,
                    burnOut=5, 
                    distanceNozzleMotorReference=1, 
                    grainNumber=1, 
                    grainDensity=1000, 
                    grainOuterRadius=10/2000, 
                    grainInitialInnerRadius=5/2000, 
                    grainInitialHeight = 0.3, 
                    oxidizerTankRadius = 127/2000, 
                    oxidizerTankHeight= 2, 
                    oxidizerInitialPressure= 60, 
                    oxidizerDensity= 500, 
                    oxidizerMolarMass= 44/1000, 
                    oxidizerInitialVolume= 1/100, 
                    distanceGrainToTank= 1/10, 
                    injectorArea=1/10000, 
                    grainSeparation=0, 
                    nozzleRadius=0.0335, 
                    throatRadius=0.0114, 
                    reshapeThrustCurve=False, 
                    interpolationMethod="linear")
# Btw I think it would be nice having some standard values for some of these arguments

and then the hybrid.info()method will return the following:
image

image

  1. I also believe we need to implement a simple unit test to guarantee next PRs will be verified using github actions

  2. Finally, the info() and allInfo() method can be improved a little bit for HybridMotor class,

@Gui-FernandesBR Gui-FernandesBR self-requested a review June 21, 2022 12:29
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.

Comments stated below, nice work anyway

An example notebook for the HybridMotor class and a simple unit test were added, the Info and AllInfo methods were complemented.
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@Gui-FernandesBR
Copy link
Member

Never saw such behaviors... we need to adrres pytest failure before merging

image

@Gui-FernandesBR Gui-FernandesBR requested review from MateusStano and removed request for giovaniceotto July 3, 2022 02:47
@Gui-FernandesBR
Copy link
Member

Thanks @MateusStano for solving the problems with github actions

@Gui-FernandesBR Gui-FernandesBR merged commit ae32e0e into hybrid-integration-v2 Jul 3, 2022
@Gui-FernandesBR Gui-FernandesBR deleted the PR-3-HM-Linear branch September 10, 2022 22:26
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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants