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 documents #2257

Merged
merged 22 commits into from
Nov 3, 2020
Merged

Improve documents #2257

merged 22 commits into from
Nov 3, 2020

Conversation

nissy-dev
Copy link
Member

@nissy-dev nissy-dev commented Oct 29, 2020

Motivation

The current documents are not readable. I think there are some problems.

First, the index.rst is too fat. I think the simpler the index pages is, the more people reads. Actually, the index page of many OSS is very simple.

Second, the TOC Tree is not good. The point is the order and the content.
About the order, the last TOC tree is the contribution guide, so it is difficult to recognize the contribution guide section. And then, I don't understand why the top of first TOC tree is Tutorial. We should put on the Installation at the top. Before trying to do tutorials, users basically need to install the package.
About the contents, some TOC Tree contents like Datasets, DataLoaders and Data classes are hard to recognize the difference for newcomers. Basically, the TOC Tree about API reference is too fat.

Third, it doesn't have local summaries (like https://lifesci.dgl.ai/api/utils.mols.html) or local contents (https://lifesci.dgl.ai/api/utils.splitters.html) through all API references. These are really helpful when users search the API which they want.

And, the all docs files are putted on docs. It is not easy to find the file we want to add docs in. Especially, requirements.txt and requirements.rst are really troublesome. Considering about increasing the documents in the future, I think we should reorganize the document directory.

You may feel these changes is trivial, but I believe it is necessary to repeat these these changes for a good document

What I did

  • Reorganize directory structure
    • the current directory structure is the same as the standard structures like DGL-Life-sci or PyG.
    • all rst files are moved to docs/source
    • docs/source has get_started, development_guide and api_reference directories and files are moved to the appropriate directory.
  • Update conf.py
    • Fix the configuration based on JAXChem and PyG
  • Update documents except API reference
    • I add the many comments in this PR.

After merging this PR, I will make the PR about updating the API reference docs step by step.
I already have updated about data, splitters, transformers in this branch, https://github.com/nd-02110114/deepchem/tree/fix-api-docs-1

Copy link
Member

@rbharath rbharath left a comment

Choose a reason for hiding this comment

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

Lots of changes here :). Let me know when this is ready for a review! I did a brief pass through, but it would be useful to have a top level listing of the changes this PR makes and the motivation for them

@@ -30,7 +30,7 @@ materials science, quantum chemistry, and biology.

## Requirements

DeepChem currently supports Python 3.5 through 3.7 and requires these packages on any condition.
DeepChem currently supports Python 3.6 through 3.7 and requires these packages on any condition.
Copy link
Member Author

Choose a reason for hiding this comment

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

DeepChem officially tested only Python 3.6 and 3.7, so I fixed.

Copy link
Member

Choose a reason for hiding this comment

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

This isn't related to this PR, but I think with the latest RDKit DeepChem also works for 3.8. We need to add that into the test suite explicitly though

@@ -5,8 +5,8 @@
# from the environment for the first two.
SPHINXOPTS ?=
SPHINXBUILD ?= sphinx-build
SOURCEDIR = .
BUILDDIR = _build
SOURCEDIR = source
Copy link
Member Author

@nissy-dev nissy-dev Oct 30, 2020

Choose a reason for hiding this comment

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

I changed the entry directories. source is the most standard in many OSS.

Copy link
Member

Choose a reason for hiding this comment

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

Good cleanup!

@@ -3,5 +3,4 @@ scikit-learn
sphinx_rtd_theme
tensorflow==2.3.0
transformers
xgboost
Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, the GDBTModel doesn't depend on the xgboost package, so I removed.

@@ -0,0 +1,20 @@
Licensing and Commercial Uses
Copy link
Member Author

Choose a reason for hiding this comment

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

This content is moved from the original index.rst

@@ -1,7 +1,16 @@
Examples
Copy link
Member Author

@nissy-dev nissy-dev Oct 30, 2020

Choose a reason for hiding this comment

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

The main modification of this file is to fix the cross-reference links like load_sampl or MultitaskRegressor

@@ -0,0 +1,52 @@
Tutorials
Copy link
Member Author

Choose a reason for hiding this comment

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

I seemed the previous tutorials is under construction.
I updates the contents by putting on the notebook links.

Copy link
Member

Choose a reason for hiding this comment

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

This expanded doctest tutorial is great! Hopefully it will provide people a nice inline code sample example for them to work with :)

@@ -0,0 +1,136 @@
The DeepChem Project
Copy link
Member Author

@nissy-dev nissy-dev Oct 30, 2020

Choose a reason for hiding this comment

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

I modified two things.

  • Make index.rst simpler (I think the simpler the index pages is, the more people reads)
    • Move Licensing and Commercial Uses to development_guide section
    • Merge Getting Involved to About US section
  • Update the outline in the bottom of this file

@nissy-dev nissy-dev changed the title [WIP] Improve documents Improve documents Oct 30, 2020
@nissy-dev
Copy link
Member Author

@rbharath
This is ready for reviews!

Comment on lines -39 to -41
autosummary_generate = True
autodoc_default_flags = ['members', 'inherited-members']
numpydoc_class_members_toctree = False
Copy link
Member Author

Choose a reason for hiding this comment

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

  • autosummary_generate is not used in docs.
  • autodoc_default_flags is the deprecated api.
  • numpydoc_class_members_toctree is not needed because we don't use numpydoc extension.

Copy link
Member

Choose a reason for hiding this comment

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

Do we need numpydock_class_members_toctree to handle the fact that our docstrings are written in numpy style? If we don't need it, good to remove!

Copy link
Member Author

Choose a reason for hiding this comment

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

numpydock_class_members_toctree is only needed by using numpydoc extension. But, the current docs use sphinx.ext.napoleon for parsing Numpy style docstring instead of numpydoc. So, I think this option is not valid, not necessary.

Copy link
Member

@rbharath rbharath left a comment

Choose a reason for hiding this comment

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

Did a first pass! I like the refactor in general. I have a few suggested tweaks

@@ -30,7 +30,7 @@ materials science, quantum chemistry, and biology.

## Requirements

DeepChem currently supports Python 3.5 through 3.7 and requires these packages on any condition.
DeepChem currently supports Python 3.6 through 3.7 and requires these packages on any condition.
Copy link
Member

Choose a reason for hiding this comment

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

This isn't related to this PR, but I think with the latest RDKit DeepChem also works for 3.8. We need to add that into the test suite explicitly though

@@ -5,8 +5,8 @@
# from the environment for the first two.
SPHINXOPTS ?=
SPHINXBUILD ?= sphinx-build
SOURCEDIR = .
BUILDDIR = _build
SOURCEDIR = source
Copy link
Member

Choose a reason for hiding this comment

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

Good cleanup!

@@ -21,39 +25,60 @@
author = 'deepchem-contributors'

# The full version, including alpha/beta/rc tags
release = '2.4.0rc'
version = deepchem.__version__
Copy link
Member

Choose a reason for hiding this comment

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

This is nice! Makes it less buggy with an automatic update

Comment on lines -39 to -41
autosummary_generate = True
autodoc_default_flags = ['members', 'inherited-members']
numpydoc_class_members_toctree = False
Copy link
Member

Choose a reason for hiding this comment

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

Do we need numpydock_class_members_toctree to handle the fact that our docstrings are written in numpy style? If we don't need it, good to remove!



# This code was borrowed from Numpy's doc-to-source linker.
# Resolve function for the linkcode extension.
Copy link
Member

Choose a reason for hiding this comment

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

This looks cleaner than the existing line linker :)

RDKit is a soft requirement package, but many useful methods like
molnet depend on it.

.. code-block:: bash
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps we should change the top level to the nightly build for now since the conda release is still at 2.3.0

Copy link
Member Author

Choose a reason for hiding this comment

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

I agreed! I'm fixing!




Datasets
Copy link
Member

Choose a reason for hiding this comment

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

I think it's still useful to have a basic tutorial in the docs itself. Colab is a little more heavy weight and it can be nice to give people a very quick glimpse of DeepChem code. Let's add this back into the new tutorial file.

Ideally, we could expand this out so it's a self-contained micro-overview of all of DeepChem :)

Copy link
Member Author

Choose a reason for hiding this comment

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

it can be nice to give people a very quick glimpse of DeepChem code

I think so, too! I will go back the dataset tutorials and add featurizer, splitter and model sections.

@nissy-dev
Copy link
Member Author

This is ready for a second review!

Copy link
Member

@rbharath rbharath left a comment

Choose a reason for hiding this comment

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

The new tutorial looks really nice! I think this is almost ready to merge in. Have a minor comment in one link where a user id got pasted in but once that's fixed feel free to merge in

@@ -167,7 +167,7 @@ If you are using the Windows and the PowerShell:


.. _`DeepChem Tutorials`: https://github.com/deepchem/deepchem/tree/master/examples/tutorials
.. _`forum post`: https://forum.deepchem.io/t/getting-deepchem-running-in-colab/81
.. _`forum post`: https://forum.deepchem.io/t/getting-deepchem-running-in-colab/81/7?u=nd-02110114
Copy link
Member

Choose a reason for hiding this comment

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

I think the copy paste here added in your user id which should be removed

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, this is a mistake. I fixed.

@@ -0,0 +1,52 @@
Tutorials
Copy link
Member

Choose a reason for hiding this comment

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

This expanded doctest tutorial is great! Hopefully it will provide people a nice inline code sample example for them to work with :)

@coveralls
Copy link

coveralls commented Nov 3, 2020

Coverage Status

Coverage increased (+0.03%) to 80.693% when pulling 4f26c4a on nd-02110114:fix-docs-build into d79c5ea on deepchem:master.

@nissy-dev
Copy link
Member Author

I fixed the links and the CI passed. So, I merge in.

@nissy-dev nissy-dev merged commit 27a8ef1 into deepchem:master Nov 3, 2020
@nissy-dev nissy-dev deleted the fix-docs-build branch November 3, 2020 14:01
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