Skip to content

Conversation

@MateusStano
Copy link
Member

Pull request type

  • Code base additions (bugfix, features)
  • Code maintenance (refactoring, formatting, renaming, tests)
  • ReadMe, Docs and GitHub maintenance
  • Other (please describe):

Pull request checklist

  • Code docs are working correctly
  • 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

Does this introduce a breaking change?

  • Yes
  • No

Info

Since we started working on v1.0 the documentation was never revised. There were a lot of inconsistency in the documentation due to the several different changes in parameters names, and the way positions related arguments are inputted.

Another important addition was the use of Jupyter Sphinx Extension. This allows us to run jupyter notebooks cell directly in the .rst files. Essentially, this means that we do not need to have notebooks for every little thing we want to document. Using notebooks for documentation is very limiting because using hyperlinks in between documentation is complicated, using admonitions is impossible, and they are pretty hard to maintain.

Jupyter Sphinx runs whenever the documentation is built, meaning we do not have to keep notebooks that have all outputs displayed and, if the Jupyter Sphinx fails to build, an error will be raised, so we do not have to manually guarantee that all notebooks are running correctly.

Ideally, we would abandon notebooks for documentation and only leave getting_started.ipynb as an example for users. However, in this PR I have only removed the motor notebooks (and converted them into .rst), since their documentation was very lacking, and I wanted to improve on it.

The Environment related notebooks are pretty well written, so I left them as documentation because this PR was already taking enough time to finish.

What was done

  • Light/Dark mode have been added along with correct displaying the RocketPy logo
  • All the docstrings of ALL classes have had slight changes (indentations, small grammar changes). This was done to correct some issues that were present in the Code Reference part of the Docs. Pretty much all classes had errors in the docs
  • Hyper links to other pages or other classes' documentation should be working (if it isn't than please let me know)
  • All mentions of other classes or methods in the documentation should be clickable and send you to that class/method docs (if it doesn't please let me know)
  • Nothing was change in Development, Technical and Flight Examples sections
  • The User Guide section received a big overhaul:
    • A First Simulation page was created, which is very similar to getting_started notebook, but with useful links to other documentation pages
    • All the Motor usage pages have been improved and now are better explained
    • The Tank and TankGeometry usage have been rewritten and are much better
    • A Positions and Coordinate Systems page has been created. It has a lot on it, and it might be one of the most important pages on the docs
    • A Rocket Usage page was added that foes into detail every step of defining a Rocket

And a lot of other minor things, and probably some more that I can't remember

Reviewing Tips

There are a lot of changed files, but the vast majority of them are just small docs changes or notebook changes.

I recommend just building the docs to review this PR.

To see the documentation completely, go into your cloned rocketpy folder and run on terminal

cd docs
make html

Essentially, everything under User Guide and Code Reference needs a very good review.

Notes

Some issues that arose while working on this PR

  • @funcify_method_decorator has no documentation

  • Environment methods with @requireis_netCDF4 are not appearing in the Code Reference documentation. Seems to have to do with this issue, that might offer the solution: Doc in decorated function not generated sphinx-doc/sphinx#3783

  • __author__ and __license__ lines that were in the beggining of every *.py file were removed

  • Class FlightPhases and Class TimeNodes dont have docstrings

  • The quality parameter from the Fluids class has been removed since it was not used anywhere (and it will probably not be used in the future)

  • Missing info and all_info methods in tank classes

  • The only notebooks that I did not fix are the two Dispersion related ones, these are very outdated and would take some extra time to fix

Copy link
Member

@Gui-FernandesBR Gui-FernandesBR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Amazing work here, @MateusStano , many thanks for your contribution!!

To summarize my comments:

  • You said you removed the __author__ entry from every file. Is there any other place where we could reference the authors names?
  • Dark mode for MATLAB tutorial isn't working so well. Is that a problem related with the matlab file conversion?
  • The "README", "First simulation" and "Getting started notebook" look a bit repetitive, don't you think? Maybe we should keep only the first simulation and the README descriptions, what do you think?
  • the favicon needs to be updated
  • For the future, we have a great challenge of removing all the notebooks and start using only .rst files for documentation. No more binary files in our repo, finally!
  • Please void to rename a file. this makes git to understand that you deleted a file and created other, thus missing the track of changes. Instead you can use git rename command to do the same thing.
  • Readthedocs is building your documentation pretty well now, it seems like a complete job for me. Approved. https://docs.rocketpy.org/en/doc-docs-overview/index.html

@Gui-FernandesBR
Copy link
Member

What is the reasoning behind this PR merging into beta/v1 instead of rel/v1.0.0a1?

This is quite a large PR. I have finished reviewing about 40% of it. Will continue tomorrow.

Here is a simple fix that can be implemented in the meantime:

  • Two sphinx extensions have been added to conf.py: sphinx_design and jupyter_sphinx. They need to be added to docs/requirements.txt in order for ReadTheDocs to be able to build the documentation successfully.

I believe the head branch is correct here. We should keep rel/v1.0.0a1 steady and without changes, including its documentation. All the work that has been done by Stano should be a part of our next release.

readthedocs was fixed too.

The only problem that I'm seeing here is the merge conflict in the Flight class. Could you solve it @MateusStano ?

@Gui-FernandesBR Gui-FernandesBR linked an issue Sep 2, 2023 that may be closed by this pull request
Copy link
Collaborator

@phmbressan phmbressan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work on the documentation. Some small corrections I noticed during the PR that I cannot comment separately, since these lines were not changed:

  • On the Rocket class, all the add_surface internal references are broken. Perhaps it should be add_surfaces;
  • The last add_fin method in the documentation of the Rocket class appears to be incomplete and does not follow standard documentation;
  • In general, many equations of the documentation would benefit from some inline LaTeX (e.g. Parachute Cds docstring);
  • Parachute class:
    • noise_signal (and similar attributes) type should be list of tuple followed by a description of the tuple content;
    • noise_signal_function, noisy_pressure_signalFunction, clean_pressure_signalFunction names/docstring have camelCase naming.
  • Flight class:
    • Documentation has an issue with the Mechanics Fluid subtitle, so that there is an incorrect link to the Fluid class;
    • In __init__ docstring of Rocket and Environment, there is no need to repeat the "rocketpy" in their description, since their types are specified right before;
    • Flight attributes, such as x, y, z would benefit a lot from a functional internal link to the Function class in their description (this happens in other classes also, but here is more important);
    • post_process method has an issue with the bullet point listing;
    • export_data and export_kml have an issue that the character '*' is interpreted as an internal link to the same page. They should be escaped.
  • EnvironmentAnalysis method converted_pressure_level_data has a typo that should be "therefore".

Something I noticed is that there are numerous internal references to code classes in the docstring description (not the types) that the parser considers as regular text.

Nonetheless these are small errors and the overall work in this PR was amazing. This list represents the issues that I caught while reading the docs and many are easy to fix, I will put a checkbox on them so everybody can coordinate the work.

@MateusStano
Copy link
Member Author

  • Dark mode for MATLAB tutorial isn't working so well. Is that a problem related with the matlab file conversion?

Seems to be something to do with including MATLAB live scripts in the documentation. I couldn't find a fix for it though

  • The "README", "First simulation" and "Getting started notebook" look a bit repetitive, don't you think? Maybe we should keep only the first simulation and the README descriptions, what do you think?

I think it is a good ideia. I removed the getting started notebook page from the docs, however I think having the getting_started notebook is still pretty useful, so I left links to it and its colab version in the First Simulation page.

@MateusStano
Copy link
Member Author

Hey! Think I changed everything that was requested here.

The tests are passing locally, but there seems to be a problem with Environment in the actions. It is unrelated to this PR though

@Gui-FernandesBR
Copy link
Member

Hey! Think I changed everything that was requested here.

The tests are passing locally, but there seems to be a problem with Environment in the actions. It is unrelated to this PR though

I reccommed not merging before solving the testing issue.
It seems to be a problem related to python 3.11.5, according to @phmbressan

FAILED tests/test_environment.py::test_era5_atmosphere - KeyError: 'pick_event'

@Gui-FernandesBR
Copy link
Member

Amazing work here! Congratulations specially to @MateusStano for pushing it hard.

Tests are passing, readthedocs is building all the pages correctly.
We are aware of some little flaws (e.g. the MATLAB dark mode), but nothing is a major issue right now.

Time to move on!

@Gui-FernandesBR Gui-FernandesBR merged commit b9cbabe into beta/v1.0.0 Sep 16, 2023
@Gui-FernandesBR Gui-FernandesBR deleted the doc/docs-overview branch September 16, 2023 02:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Docs Docs and examples related

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rocket Class Notebook still lacking!

5 participants