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

Improve documentation #6307

Merged
merged 23 commits into from
Aug 17, 2023
Merged

Conversation

saurabheights
Copy link
Contributor

@saurabheights saurabheights commented Aug 12, 2023

Type

  • Bug fix (non-breaking change which fixes an issue): Fixes build warnings, missing docs, mostly cleanup.
  • New feature (non-breaking change which adds functionality). Resolves #
  • Breaking change (fix or feature that would cause existing functionality to not work as expected) - The items order in documentation layout changes slightly. I will comment wherever these changes are made today. #

Motivation and Context

Reviewing documentation and required cleanup. This PR only address some of the issues. Each issue is fixed in single commit, so should be easier to review commit-wise. More changes will follow along in a separate PR.

P.S. My end goal is to later on address issues like architecture documentation request, fix existing documentation issues as one fixed earlier here, etc.

Checklist:

  • I have run python util/check_style.py --apply to apply Open3D code style
    to my code.
  • This PR changes Open3D behavior or adds new functionality.
    • Both C++ (Doxygen) and Python (Sphinx / Google style) documentation is
      updated accordingly.
    • I have added or updated C++ and / or Python unit tests OR included test
      results
      (e.g. screenshots or numbers) here.
  • I will follow up and update the code if CI fails.
  • For fork PRs, I have selected Allow edits from maintainers.

Description

I will add screenshot commit wise wherever necessary.


This change is Reviewable

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@@ -144,9 +144,7 @@ demonstrate the usage of Open3D Python interface. See ``examples/python`` for
all Python examples.

.. note:: Open3D's Python tutorial utilizes some external packages: ``numpy``,
``matplotlib``, ``opencv-python``. OpenCV is only used for reconstruction
Copy link
Contributor Author

@saurabheights saurabheights Aug 12, 2023

Choose a reason for hiding this comment

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

Removed missing shell script, as well as why Opencv is used (most users dont care about that) and also its easy to make this doc redundant when adding a new example where opencv is used (which might have already happened, I do need to check if numpy, matplotlib, opencv-python are the only needed libs. Maybe adding etc. and some requirements_example.txt file would be better.

docker
arm
open3d_ml
Copy link
Contributor Author

@saurabheights saurabheights Aug 12, 2023

Choose a reason for hiding this comment

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

I moved open3d_ml to the bottom and arm above, with docker option closer to right after compilation, since new user would want to see how to get started with open3d faster. Let me know if this is alright or not.

@@ -88,3 +71,20 @@ Open3D: A Modern Library for 3D Data Processing
python_example/pipelines/index
python_example/utility/index
python_example/visualization/index

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As previous commit, this one focuses on putting all python related stuff together, i.e. Tutorials, Python API and Python Examples. C++ API after Python and Contribution in the end.

Screenshot from 2023-08-12 18-01-55

Screenshot from 2023-08-12 18-02-09

@@ -405,7 +405,7 @@
"cell_type": "markdown",
"metadata": {},
"source": [
"#### 1. Set Inputs and Parameters"
"### 1. Set Inputs and Parameters"
Copy link
Contributor Author

@saurabheights saurabheights Aug 12, 2023

Choose a reason for hiding this comment

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

More info on why here - spatialaudio/nbsphinx#373 (comment)

P.S. Just noticed that the changes here are getting over written somehow when building docs. Will check and fix this commit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Found my oversight. So, changes here are getting ignored because a pre-built t_icp_registration notebook file is getting downloaded in make_docs.py due to #4321.

Is the documentation still built without cuda on CI/CD?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think so. It is safe to keep the changes here now, we can study the best option regarding large notebook files in another PR.

…d-string.`

This escapes vertical bars in bash tree output in pydocs but doesnt fix rendering in html.
docs/conf.in.py Outdated
def setup(app):
app.connect("autodoc-skip-member", skip)
app.connect("autodoc-process-docstring", escape_vertical_bars)
Copy link
Contributor Author

@saurabheights saurabheights Aug 12, 2023

Choose a reason for hiding this comment

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

The issue comes from vertical bars in python docs which are from tree command - http://open3d.org/docs/release/python_api/open3d.data.RedwoodIndoorLivingRoom1.html. This change fixes the warning but the output is in html paragraph tag and new lines get ignored.

Screenshot from 2023-08-12 18-26-51

There was an alternate option to also remove output of tree command from docs, but I thought better keep tree output. Python docs are nicer, even if sphinx is not yet. Any ideas on how to fix for sphinx as well?

Copy link
Member

Choose a reason for hiding this comment

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

Hi @saurabheights this issue actually originates from C++ code where the doc string is written, so the best way to fix this is there. We should not need a python postprocessing script here. The doscstring is rst and the rst syntax for literal text is:

https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#literal-blocks

(double colon and indented block)

Can you switch the pybind docstring to this syntax and see if that fixes this issue?

The pybind docstring is here:

https://github.com/isl-org/Open3D/blob/master/cpp/pybind/data/dataset.cpp#L979

Similarly, the C++ doxygen documentation is also mis-formatted:

http://www.open3d.org/docs/latest/cpp_api/classopen3d_1_1data_1_1_redwood_indoor_living_room1.html#details

Doxygen uses markdown syntax for code blocks:
https://www.doxygen.nl/manual/markdown.html#mddox_code_blocks

Here is the C++ documentation:

https://github.com/isl-org/Open3D/blob/master/cpp/open3d/data/Dataset.h#L726

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ssheorey Thanks for catching this. Learnt something new. Reverted my earlier postprocessing script and added suggested solution. Worked nicely for both sphinx and doxygen.

@theNded theNded self-requested a review August 13, 2023 18:06
Copy link
Contributor

@theNded theNded left a comment

Choose a reason for hiding this comment

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

Thanks for the kind contribution!

LGTM with minor change requests. I will also have to study how to render the tree structure correctly and will post my feedback later.

docs/conf.in.py Outdated Show resolved Hide resolved
docs/tutorial/geometry/index.rst Show resolved Hide resolved
docs/make_docs.py Outdated Show resolved Hide resolved
docs/make_docs.py Outdated Show resolved Hide resolved
@@ -405,7 +405,7 @@
"cell_type": "markdown",
"metadata": {},
"source": [
"#### 1. Set Inputs and Parameters"
"### 1. Set Inputs and Parameters"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think so. It is safe to keep the changes here now, we can study the best option regarding large notebook files in another PR.

docs/conf.in.py Outdated
def setup(app):
app.connect("autodoc-skip-member", skip)
app.connect("autodoc-process-docstring", escape_vertical_bars)
Copy link
Member

Choose a reason for hiding this comment

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

Hi @saurabheights this issue actually originates from C++ code where the doc string is written, so the best way to fix this is there. We should not need a python postprocessing script here. The doscstring is rst and the rst syntax for literal text is:

https://www.sphinx-doc.org/en/master/usage/restructuredtext/basics.html#literal-blocks

(double colon and indented block)

Can you switch the pybind docstring to this syntax and see if that fixes this issue?

The pybind docstring is here:

https://github.com/isl-org/Open3D/blob/master/cpp/pybind/data/dataset.cpp#L979

Similarly, the C++ doxygen documentation is also mis-formatted:

http://www.open3d.org/docs/latest/cpp_api/classopen3d_1_1data_1_1_redwood_indoor_living_room1.html#details

Doxygen uses markdown syntax for code blocks:
https://www.doxygen.nl/manual/markdown.html#mddox_code_blocks

Here is the C++ documentation:

https://github.com/isl-org/Open3D/blob/master/cpp/open3d/data/Dataset.h#L726

Copy link
Member

@ssheorey ssheorey left a comment

Choose a reason for hiding this comment

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

Thanks @saurabheights ! This looks good!

@ssheorey ssheorey merged commit c7718fd into isl-org:master Aug 17, 2023
36 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants