-
-
Notifications
You must be signed in to change notification settings - Fork 176
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
DOC: Monte carlo documentation updates #607
DOC: Monte carlo documentation updates #607
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #607 +/- ##
===========================================
+ Coverage 73.58% 73.60% +0.01%
===========================================
Files 70 70
Lines 10313 10289 -24
===========================================
- Hits 7589 7573 -16
+ Misses 2724 2716 -8 ☔ View full report in Codecov by Sentry. |
…b.com/RocketPy-Team/RocketPy into doc/monte-carlo-documentation-updates
…nd total CPU time
0ae1c37
to
1e561fe
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.
Outstanding work @Gui-FernandesBR! I read through everything and most of the comments I had were similar to the ones made by Stano.
Sorry if my review seems lacking in comments given the size of the PR, but I believe the work here is rather solid, specially the page on Stochastic objects
.
@phmbressan @MateusStano the last commit cf24129 polished a few TODO messages (they are less "aggressive" now). I think it is ready to be merged. Can you approve please? |
Pull request type
Checklist
black rocketpy/ tests/
) has passed locallypytest tests -m slow --runslow
) have passed locallyCHANGELOG.md
has been updated (if relevant)Current behavior
The
MonteCarlo
class is ready to be deployed but there is still a room to improve its documentation and examples.New behavior
See the commit history one by one.
To keep in mind:
# this is a comment
when it is really necessary or helps to organize.Important: There is something broken with the Environment class when integrating with NOAAs data server. I cannot not solve it immediately. If you want to build the documentation locally, please comment the examples that use GFS or GEFS environment model.
Breaking change
Additional information
Still needs to accomplish:
StochasticModel
reviewMake no mistake, updating docstrings is not an easy task. I think we should release the next version using the "as-is" state, and keep improving the documentation based on users feedbacks.