Enh/dispersion class/review

[Gui-FernandesBR]

Pull request type

Please check the type of change your PR introduces:

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

I am addressing some of the problems that I so on the #232 , here is a list of my changes at the Dispersion.py files only:

• I am moving invertedHaversine function from utilities to supplement.py (this file will be renamed in the v1.0.0)
• I refactores the xxxxx_inputs dictionaries and saved them as attributes of Dirpersion class
• Erased unused classCheck method
• The old setDistributionFunc now is private since the user is not supposed to interact with it. I'm also adding docstring to it and converting to snake_case just to facilitate the visualization at this part of the code.
• The old processDispersionDict is also private now. But it consolidate several other __process_xxx_from_dict private methods. Separating the code this way, I think it is more readable.
• PLease notice that now the Dispersion.py can work with n aerodynamicsurfaces at a time.
• The new __check_inputted_values_from_dict will fill the dispersion dictionary with (mean, stdev) values where it does not have already these two parameters.
• On the other hand, __check_initial_objects will create rocketpy main objects if the user do not provide it.
• yield_flight_setting is also private now, same reasons as above. I added its docstring, please check this part because I'm not entirely sure about it.
• export_flight_data also private, but here one significant change: the variables argument can be used to chose what output variables are going to be saved from each simulation, this improves the code in terms of
• Regarding the run_dispersion method, which is a core of Dispersion.py, here are some considerations:
  • Environment is now optional, since the user can provide a flight object and the code will use the same env from this flight.
  • I am back with the environment object modifications inside the loop, and I believe it is wokring properly.
• After that, the git diff may no longer not help the reader. But basically I converted all those "plotxxxxx" methods into a single function: plot_results(). The same with the prints.
• Before the above step, you're going to find import_results(), which is working almost te same way before, it can be used without the run_dispersion method. Also, I added the __process_results(), which will now save the mean and st. dev of each output variable and an attibute of the dispersion class, allowing easy access in the future!

Does this introduce a breaking change?

• Yes
• No

Remaning tasks (at this PR):

• Update Dispersion class main docstring
• Check code speed, verifying we are not consuming time with unecessary operations.
• Check code under different scenarios (ballistic, 2 parachutes, 1 parachute, etc.)
• Double check ALL then except errors messages to ensure we are not using poor except: cases

Remaning tasks (probably in a future PR):

• Add unit tests
• Implement functions from compareDispersions notebook
• Test simulations under different scenarios (with both parachutes, with only main chute, etc)
• Allow each parameter to be varied following an specific probability distribution
• Add more options of motor (i.e. Liquid and Hybrids)
• Allow elliptical fins instead of only trapezodal
• Add initialSolution flight option
• At the print_results() method, print as a table instead of prints, this would be more organized
• Create evolution plots to analyze convergence
• In the future, integrate with other libraries to plot the map instead of loading a google earth screenshot (e.g. cartopy, ee, etc.)

Obs.: Yes, go ahead and test the dispersion_class notebook, it should be working correctly!!

[Gui-FernandesBR]

I think that the
ImportError: cannot import name 'cached_property' from 'functools'
error is related with python 3.7 version.

rumors that cached_property was introduced only on python 3.8.
Needs investigation!

[Gui-FernandesBR]

I think that the ImportError: cannot import name 'cached_property' from 'functools' error is related with python 3.7 version.

rumors that cached_property was introduced only on python 3.8. Needs investigation!

This are being addressed by #283, let's keep it on track.

[Gui-FernandesBR]

@MateusStano I have just updated the PR description including in details what I have changed here in addition to the #232 PR.

Reviewing this will definetely not be easy. I am available once you become ready to start work on dispersions again, I think we can arrange a meeting just to discuss it.

Also, with regards to @cached_properties usage, we need to wait for #283 before getting approve at the tests again! But if you use python > 3.8, code might already be working properly.

[review-notebook-app]

Check out this pull request on 

See visual diffs & provide feedback on Jupyter Notebooks.

---

Powered by ReviewNB

[MateusStano]

Hey! So given how dependent this one is on other PRs I decided to clean it up before we merge those changes here. I changed some things that either optimized the simulation or solved some bugs. It should be pretty self-explanatory just by looking at the commits.

I left a lot of TODOs with what we still need to do. There are also some bugs happening, if you try to run the 2nd example on the dispersion_class_usage notebook none of the rockets actually leave the ground. Probably some issues when defining the motor but I don't think it is worth fixing It yet. It is better to wait for #282 to be done to continue.

I think this review is better than the version in the original Dispersion PR and since we have to wait for other PRs there are a couple of ways we continue developing this:

• Continue developing the Dispersion class in this branch and PR

  Or

• Merge this branch with the original PR and go from there ( I prefer this way)

What do you think we should do @Gui-FernandesBR ?

[MateusStano]

Oh and by the way. I think all the Remaining Tasks you commented in this PR main initial comment are all still in need to be done, but I think they should all be things we open other PRs for implementations, instead of just committing directly to this one

[Gui-FernandesBR]

Hey! So given how dependent this one is on other PRs I decided to clean it up before we merge those changes here. I changed some things that either optimized the simulation or solved some bugs. It should be pretty self-explanatory just by looking at the commits.

I left a lot of TODOs with what we still need to do. There are also some bugs happening, if you try to run the 2nd example on the dispersion_class_usage notebook none of the rockets actually leave the ground. Probably some issues when defining the motor but I don't think it is worth fixing It yet. It is better to wait for #282 to be done to continue.

I think this review is better than the version in the original Dispersion PR and since we have to wait for other PRs there are a couple of ways we continue developing this:

• Continue developing the Dispersion class in this branch and PR
  Or
• Merge this branch with the original PR and go from there ( I prefer this way)

What do you think we should do @Gui-FernandesBR ?

As you prefer!
tks for reviewing,
Still some issues to be solved, but I agree that we should go step by step.