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: Introducing the Monte Carlo class #232

Merged
merged 383 commits into from
May 21, 2024
Merged

Conversation

MateusStano
Copy link
Member

@MateusStano MateusStano commented Sep 16, 2022

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 current behavior?

Currently, the dispersion analysis is only made inside notebooks , requiring different code blocks to be defined every time you set a new Monte Carlo analysis.

What is the new behavior?

The MonteCarlo is added as a class in the code. This is continuing what was developed in the 2022 RocketPy Hackathon at RocketPy-Team#106

Does this introduce a breaking change?

  • No! All the previous code should be working.

Other Information:

  • This is by far the largest PR we have ever done. A lot of mistakes were introduced and fixed after months. It has been a really try-wait-improve approach.
  • Performance: Each simulation is consuming 4-5 seconds, demanding a total time of about 1hour to run 1000 simulations. We should pursue a x4 speed up in future PR (please: don't use this PR to commit speed up improvements, let's finish this first and then work on improvements).
  • Tests and docs can still be improved, but the bare minimum to be merged/released is already done. Therefore, reviewers can already start working on this one!

@MateusStano MateusStano added the Enhancement New feature or request, including adjustments in current codes label Sep 16, 2022
@Gui-FernandesBR Gui-FernandesBR changed the base branch from master to develop September 22, 2022 12:37
@Gui-FernandesBR Gui-FernandesBR marked this pull request as ready for review September 27, 2022 21:26
@Gui-FernandesBR Gui-FernandesBR removed the request for review from FranzYuri September 27, 2022 21:26
@Gui-FernandesBR Gui-FernandesBR removed this from the RocketPy at EuroC 2022 milestone Oct 24, 2022
@MateusStano MateusStano added this to the Release v1.0.0 milestone Oct 30, 2022
@Gui-FernandesBR Gui-FernandesBR mentioned this pull request Nov 3, 2022
36 tasks
@Gui-FernandesBR
Copy link
Member

I've updated the task list here (at PR description), but please notice that there's also another huge tasklist at #267 coming!

@Gui-FernandesBR Gui-FernandesBR changed the title WIP: ENH: Class Dispersion ENH: Introducing the Dispersion class Nov 12, 2022
@Gui-FernandesBR Gui-FernandesBR added the Monte Carlo Monte Carlo and related contents label Nov 15, 2022
@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@MateusStano MateusStano changed the base branch from develop to beta/v1.0.0 January 16, 2023 02:25
@Gui-FernandesBR
Copy link
Member

While trying to refactor my PR to use the Monte Carlo class, one thing I found missing is to be able to export the trajectories. For the notebook mentioned, this information is not critical and I can omit its use. @MateusStano, @Gui-FernandesBR, @phmbressan, do you think this information would be important for future use of the class? Exporting the trajectories is a bit harder since it requires some "convention." What I did in my rough first implementation was to fix a sampling rate and sample all the trajectories at multiples of that time step from 0 to the t_final of the reference trajectory. In our case, this upper bound could be max(t_final_array) where t_final_array is an array of all t_final of all trajectories sampled. That would require, however, to store all flight objects until the end.

I believe this is something we do in the future, not so urgent. At a first glance, I think saving the trajectory positions coordinates may be too expensive.

@Gui-FernandesBR
Copy link
Member

After 375 commits and more than 600 days, this PR is finally ready for a last review and merge.

Calling out all the reviewers @RocketPy-Team/code-owners so we can have a last double-check in this code before going to the develop branch.

@MateusStano
Copy link
Member Author

MateusStano commented May 16, 2024

Hey! I noticed quite a lot of important documentation was missing so I took the liberty to change a lot of the usage notebook

It is still very far from being good I think, but I hope things are better explained now. It is very important that someone reviews what I wrote in there carefully, and improves on anything that needs it

There are still a couple of problems I found

  • When you write the name of a stochastic object in the last line of a cell in the notebook, it gets the __repr__ of the class, which is not very useful. I would say leaving the __repr__ as what now is the __str__ would be really important
  • The __str__ of a StochasticRocket with added motor, aerosurfaces, and/or parachutes is a mess right now
  • Still need to add an explanation on how to add an image to dispersion.plot.ellipses and how to translate that image in the plot
  • Need to add an explanation of how to set the export_list of the MonteCarlo class and how these are saved on the files. Also, still needs to add docs explaining what the default export_list is and what are the possible exportable variables
  • The export_ellipses_to_kml raises an error for me

@Gui-FernandesBR
Copy link
Member

Hey! I noticed quite a lot of important documentation was missing so I took the liberty to change a lot of the usage notebook

It is still very far from being good I think, but I hope things are better explained now. It is very important that someone reviews what I wrote in there carefully, and improves on anything that needs it

https://www.productplan.com/glossary/minimum-viable-product/

There are still a couple of problems I found

  • When you write the name of a stochastic object in the last line of a cell in the notebook, it gets the __repr__ of the class, which is not very useful. I would say leaving the __repr__ as what now is the __str__ would be really important

I think I temporarily addressed this issue here: 521a8e8
Can be improve in future PRs.

  • The __str__ of a StochasticRocket with added motor, aerosurfaces, and/or parachutes is a mess right now
    The problem is that a __str__ ideally is not so complex as we have today. You can see as an example the Function class, which has a really simple "str". If you want so badly a method to visualize what is inside a StochasticModel, then we should probably add a method: "your_stochastic_object.visualize_variables()" or similar names.
  • Still need to add an explanation on how to add an image to dispersion.plot.ellipses and how to translate that image in the plot

Can be done in the future. I don't really know if we should allow the user to input an image or simply download an satellite image at the moment we run the function. Let it to the future.

  • Need to add an explanation of how to set the export_list of the MonteCarlo class and how these are saved on the files. Also, still needs to add docs explaining what the default export_list is and what are the possible exportable variables

This requires time and effort, let's leave it to the next PR of documentation.

  • The export_ellipses_to_kml raises an error for me

What error? How can we reproduce it? ...
It is running flawlessly in my machine, including the tests.
Also, this is an extra feature. Can be fixed in another PR.

@Gui-FernandesBR
Copy link
Member

Ok, thank you for the review and the updates in the .ipynb file, @MateusStano .

As agreed, let's follow through this PR, approve and merge it. I'm working on documentation improvements and this will be addressed in a new PR.

@MateusStano
Copy link
Member Author

Awesome! This is finally ready to be merged. Probably the biggest PR we ever had, and we never make anything as big as this again heheheh

@Gui-FernandesBR Gui-FernandesBR merged commit e7b730a into develop May 21, 2024
10 checks passed
@Gui-FernandesBR Gui-FernandesBR deleted the enh/class_dispersion branch May 21, 2024 22:52
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 Monte Carlo Monte Carlo and related contents Refactor
Projects
Status: Closed
Development

Successfully merging this pull request may close these issues.

8 participants