-
-
Notifications
You must be signed in to change notification settings - Fork 168
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: Implementing Multivariate Rejection Sampling (MRS) in RocketPy #738
base: develop
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #738 +/- ##
===========================================
- Coverage 76.42% 75.92% -0.50%
===========================================
Files 95 97 +2
Lines 11090 11198 +108
===========================================
+ Hits 8475 8502 +27
- Misses 2615 2696 +81 ☔ View full report in Codecov by Sentry. |
This PR is ready for a first "Design Review." I would like to get your opinion if this implementation provides what you think on how the user should use the MRS. Albeit a implementation as a function seems natural, I implemented as a class because:
It currently works as follows:
I provided a quick and dirty notebook, which will be removed, just to show how the class is being used at the moment. |
766bd58
to
c45fabf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't have much time so I'll be short
- The class dies after you sample the data once, this makes it pointless to have a class
- Instead I'd remove the sample dictionary from arguments.
- First read the data during initialization. Then you use a function to set the variables you are going to allow be varied. This way u can already anticipate which variables may be varied.
- We want almost instant results for a MonteCarlo simulation after using MRS.
- Other thing is that the user must supply the original pdf. Could we possile estimate this from data? (imagine 70k)
- Finally, plotting is crucial for MRS, or even tables. We'd love to see more on that later.
c45fabf
to
d3ab5f2
Compare
f"the monte carlo input file {input_filename}!" | ||
) from e | ||
|
||
input_file.close() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line should be inside a finally
block, I believe. The same applies to the method load_output
.
"""Loads input information from monte carlo in a SampleInformation | ||
object | ||
""" | ||
input_filename = f"{self.monte_carlo_filepath}.inputs.txt" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If possible, it is better practice to handle paths with the pathlib
standard library. Check out the parallel monte carlo PR as an example of handling the filename
in the MonteCarlo class.
Pull request type
Checklist
black rocketpy/ tests/
) has passed locallypytest tests -m slow --runslow
) have passed locallyCHANGELOG.md
has been updated (if relevant)New behavior
This PR implements the MRS requested in #162 and described in RocketPy paper.
Breaking change
Additional information
It is possibly a good idea to use setters when
distribution_dict
is modified by the user. Otherwise, thesample
method can provide incorrect results due to attributes computed in the initialization.