ENH: burnout time changes. Breaking changes

[FranzYuri]

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?

burnout time is a required variable

What is the new behavior?

Now the variable is called burn time and can be a tupple with starting and ending times.

Does this introduce a breaking change?

The only beaking change is that the name of the variable burnOut changed to burn_time

• Yes
• No

Other information

Enter text here...

[MateusStano]

Hey, I just commited a few big changes in the code you had made. First I added the use of Function.setDiscrete to remove the while loops. Then I improved some of the if else structure to have no repetitions.

There is one big problem still. When using the burn_time as tuple the propellant mass does not go to zero.
Here is an example, using the Calisto thrust curve with burn_time = (1,3) the thrust curve is:

And the propellant mass:

@phmbressan can you confirm this is a problem and maybe fix it? I think you will understand what is going on here much faster than me.

[giovaniceotto]

• Document new attributes

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[phmbressan]

@MateusStano

The issue of negative propellant mass is fixed now. As you can see in my earlier commits, the main cause were some methods that still considered all burns starting at zero seconds.

On top of that, there were some issues with the type checking and conversion for the parameter burn_time, I changed it a bit. It is not the most elegant code since a wide range of input types must checked and converted, but I believe it is working. As always, additional reviews of these changes are important.

[Gui-FernandesBR]

What do we need to do in order to get this PR ready for review?

Btw shouldn't it go straight to the beta/v1.0.0 branch?

[MateusStano]

@phmbressan to merge this one we need two more things:

• Change motor.thrust to only take into account the thrust curve during burn time
• Merge enh/liquid-motors branch into this one

[phmbressan]

There are still some issues regarding this PR:

• The tests for SolidMotor are extremely tight in some cases (e.g. tolerances of 1e-16), thus, due to the new usage of a termination event in grain geometry, there are tests that do not pass.
• Perhaps some revisions of nomenclature and definitions of old methods, such as reshapeThrustCurve, may be necessary. I am not sure why the test for this method also does not pass.

[phmbressan]

@MateusStano Thank you for your suggestions. I believe the issues you raised were solved, however another review is always important, so I ask you to review these latest changes.

[giovaniceotto]

IMPORTANT: I have reviewed only rocketpy.motors.Motor.reshapeThrustCurve().

Two simple issues need to be addressed. Once they have been solved, feel free to automatically consider this approved.

[phmbressan]

Sorry for submitting a new review, I had an issue with github pending comments.

The tests are not passing due to the really odd lack of a library in github actions, I have not been able to comprehend why this is happening. Locally they are fine for me.

[Gui-FernandesBR]

Sorry for submitting a new review, I had an issue with github pending comments.

The tests are not passing due to the really odd lack of a library in github actions, I have not been able to comprehend why this is happening. Locally they are fine for me.

What library? Can you describe it better?

[phmbressan]

Sorry for submitting a new review, I had an issue with github pending comments.
The tests are not passing due to the really odd lack of a library in github actions, I have not been able to comprehend why this is happening. Locally they are fine for me.

What library? Can you describe it better?

From the testing log in github actions.

../../../hostedtoolcache/Python/3.7.17/x64/lib/python3.7/bz2.py:19: in <module>
    from _bz2 import BZ2Compressor, BZ2Decompressor
E   ModuleNotFoundError: No module named '_bz2'

I searched a bit and it seems related to the pandas import in ndrt_2020 testing. This is odd, since those were not altered and are alright in other branches.

Edit: this seems to be a known issue of github actions. After updating and merging the liquid motors branch it was fixed with the bump to Python 3.8. The tests are passing now.

[Gui-FernandesBR]

Great solving!