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

DOC: Documentation overhaul #3124

Merged
merged 23 commits into from
Jan 2, 2020
Merged

Conversation

oesteban
Copy link
Contributor

@oesteban oesteban commented Dec 29, 2019

Building on top of #3119 and #3120, this PR makes a deep revision of the
documentation:

  • Added a new build_docs job to CircleCI to test how it renders.

  • Minimized external machinery (under /tools/) when building the
    documentation:

    1. Some minimal modifications to sphinx extensions (apidoc,
      napoleon) allow the generation of special documentation
      for nipype interfaces, as it used to be before this PR
    2. A new sphinx extension (nipype.sphinxext.apidoc) takes
      care of parsing and rendering inputs and outputs. They now
      look like the parameters/arguments of functions when formatted
      with numpydoc.
  • Revised the description of many interfaces and the documentation of
    the main class and the input/output specs.

  • Revised the structure of the navbar, separating out
    User-Guide/Examples, Interfaces-Index, and Devs' documentation.

  • Minimized the number of WARNINGS at documentation build to 5 (4 of
    them coming out from the auto-generated SEM tools).

Building on top of nipy#3119 and nipy#3129, this PR makes a deep revision of the
documentation:

  * Added a new ``build_docs`` job to CircleCI to test how it renders.
  * Minimized external machinery (under ``/tools/``) when building the
    documentation:

      1. Some minimal modifications to sphinx extensions (apidoc,
         napoleon) allow the generation of special documentation
         for nipype interfaces, as it used to be before this PR
      2. A new sphinx extension (``nipype.sphinxext.apidoc``) takes
         care of parsing and rendering inputs and outputs. They now
         look like the parameters/arguments of functions when formatted
         with numpydoc.

  * Revised the description of many interfaces and the documentation of
    the main class and the input/output specs.
  * Revised the structure of the navbar, separating out
    User-Guide/Examples, Interfaces-Index, and Devs' documentation.
  * Minimized the number of WARNINGS at documentation build to 5 (4 of
    them coming out from the auto-generated SEM tools).
@codecov
Copy link

codecov bot commented Dec 29, 2019

Codecov Report

Merging #3124 into master will decrease coverage by 1.05%.
The diff coverage is 39.39%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3124      +/-   ##
==========================================
- Coverage   65.01%   63.95%   -1.06%     
==========================================
  Files         295      297       +2     
  Lines       39290    39408     +118     
  Branches     5178     5210      +32     
==========================================
- Hits        25543    25202     -341     
- Misses      12692    13162     +470     
+ Partials     1055     1044      -11
Flag Coverage Δ
#unittests 63.95% <39.39%> (-1.06%) ⬇️
Impacted Files Coverage Δ
nipype/interfaces/dipy/tensors.py 39.72% <ø> (ø) ⬆️
nipype/interfaces/cmtk/cmtk.py 17.96% <ø> (ø) ⬆️
nipype/interfaces/niftyseg/maths.py 77.52% <ø> (-0.25%) ⬇️
nipype/interfaces/freesurfer/registration.py 78.07% <ø> (ø) ⬆️
nipype/interfaces/vista/vista.py 100% <ø> (ø) ⬆️
nipype/info.py 87.69% <ø> (-4.62%) ⬇️
nipype/interfaces/minc/minc.py 85.17% <ø> (ø) ⬆️
nipype/interfaces/cmtk/parcellation.py 12.5% <ø> (ø) ⬆️
nipype/interfaces/dtitk/__init__.py 100% <ø> (ø) ⬆️
nipype/algorithms/confounds.py 67.18% <ø> (ø) ⬆️
... and 102 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92d8bc3...e74d33b. Read the comment docs.

@oesteban oesteban force-pushed the doc/sphinxext-interfaces branch 4 times, most recently from bbc200b to d8699f7 Compare December 29, 2019 23:24
@oesteban
Copy link
Contributor Author

Et voilà, built!

One of the most salient improvements is that now, one can access an overview of all the interfaces within a package (regardless of which module contains it) - e.g. AFNI

The interfaces index has also been substantially improved, adding descriptions for all tools wrapped.

@satra
Copy link
Member

satra commented Dec 30, 2019

@oesteban - overall this looks great (and feel free to merge).

a few thoughts on structure and refactor (all these could be done in subsequent PRs):

  • currently there are similar interface api pieces for doc generation, make specs, and nipype2boutiques/cli run (under the click related functions). the interface walkthrough and parse functions could potentially be aggregated for doc generation, nipype2boutiques/cli, and for migration from nipype2pydra interfaces.

  • we should see what it would take to make the sphinx html responsive (it does not change under different device formats)

  • i love the "what tools are wrapped" addition. it would be nice if this could be standardized at the package/module level, with some iterator generating the list. as we think about migration into separate repos, we may have to start thinking about a package registry format.

  • a few classes may still need to be eliminated from doc generation (have not done an exhaustive search): TVTKBaseInterface, CommandLineDtitk, ... (as such there may be differences between an exhaustive walkthrough and a filtered walkthrough)

  • we should add to the html frontpage a temporary banner that nipype is now python 3 only (and the last branch to support 2 is 1.3.x)

@satra
Copy link
Member

satra commented Dec 30, 2019

and there was this failure: https://readthedocs.org/projects/nipype/builds/

image

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

This looks very good. Thanks! I've made some comments, but it will take me a while to get through 119 changed files. If it makes sense to merge and fix issues later as they're found, that's fine with me, as it's overall a huge improvement.

doc/interfaces.rst Outdated Show resolved Hide resolved
doc/examples.rst Show resolved Hide resolved
doc/interfaces.rst Show resolved Hide resolved
doc/documentation.rst Show resolved Hide resolved
doc/make.bat Show resolved Hide resolved
nipype/algorithms/confounds.py Outdated Show resolved Hide resolved
nipype/algorithms/modelgen.py Show resolved Hide resolved
nipype/algorithms/modelgen.py Show resolved Hide resolved
nipype/interfaces/base/support.py Outdated Show resolved Hide resolved
nipype/interfaces/base/support.py Outdated Show resolved Hide resolved
Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Another batch of comments.

nipype/interfaces/base/core.py Outdated Show resolved Hide resolved
nipype/interfaces/base/core.py Outdated Show resolved Hide resolved
nipype/interfaces/cmtk/cmtk.py Outdated Show resolved Hide resolved
nipype/interfaces/cmtk/cmtk.py Outdated Show resolved Hide resolved
nipype/interfaces/cmtk/cmtk.py Outdated Show resolved Hide resolved
nipype/interfaces/cmtk/parcellation.py Outdated Show resolved Hide resolved
nipype/interfaces/cmtk/parcellation.py Outdated Show resolved Hide resolved
nipype/interfaces/diffusion_toolkit/dti.py Outdated Show resolved Hide resolved
nipype/interfaces/diffusion_toolkit/odf.py Outdated Show resolved Hide resolved
nipype/interfaces/diffusion_toolkit/dti.py Outdated Show resolved Hide resolved
@oesteban
Copy link
Contributor Author

oesteban commented Jan 1, 2020

@satra

and there was this failure: https://readthedocs.org/projects/nipype/builds/

Can you give me admin perms in readthedocs? We just need to update the pointer to dependencies.

  • currently there are similar interface api pieces for doc generation, make specs, and nipype2boutiques/cli run (under the click related functions). the interface walkthrough and parse functions could potentially be aggregated for doc generation, nipype2boutiques/cli, and for migration from nipype2pydra interfaces.

Sorry, I think I don't understand what you mean here.

  • we should see what it would take to make the sphinx html responsive (it does not change under different device formats)

I guess that's just a matter of the theme. If we want to keep current, then it will be a bit more involved.

  • i love the "what tools are wrapped" addition. it would be nice if this could be standardized at the package/module level, with some iterator generating the list. as we think about migration into separate repos, we may have to start thinking about a package registry format.

I started setting up the comment for each tool as short description for each module, but finally gave up for time concerns. I agree this would be great to automate, and setting is as the docstring of each package's __init__.py (or module if applies) seems the right way (and easiest) way forward.

  • a few classes may still need to be eliminated from doc generation (have not done an exhaustive search): TVTKBaseInterface, CommandLineDtitk, ... (as such there may be differences between an exhaustive walkthrough and a filtered walkthrough)

I think I added a sphinx's config.py hook for these. Finally, the PR became so large I don't know anymore if that intention is implemented. Would you like me to double-check?

  • we should add to the html frontpage a temporary banner that nipype is now python 3 only (and the last branch to support 2 is 1.3.x)

Sure, that I think is beyond the scope of this PR, but I could include it - if you want.

@effigies

This looks very good. Thanks! I've made some comments, but it will take me a while to get through 119 changed files. If it makes sense to merge and fix issues later as they're found, that's fine with me, as it's overall a huge improvement.

Thanks! I apologize for this huge PR, but after some consideration, I thought it was better this way -- Having merged this with a quick look around here and there, and then accepting more nitty-gritty changes addressing those problems we left open.

I'll get your suggestions merged ASAP, and then we can build from there. I believe the biggest plus of this PR is actually outside nipype proper - other tools that define interfaces (e.g., niworkflows) will have it really easy to generate proper docs.

Thank you both for all the feedback!

Co-Authored-By: Chris Markiewicz <[email protected]>
Copy link
Contributor Author

@oesteban oesteban 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 feedback!

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Okay, got through all of them.

examples/fmri_spm_auditory.py Outdated Show resolved Hide resolved
examples/fmri_spm_auditory.py Outdated Show resolved Hide resolved
nipype/algorithms/confounds.py Outdated Show resolved Hide resolved
nipype/algorithms/confounds.py Outdated Show resolved Hide resolved
nipype/algorithms/modelgen.py Show resolved Hide resolved
nipype/interfaces/spm/model.py Show resolved Hide resolved
nipype/interfaces/spm/model.py Outdated Show resolved Hide resolved
nipype/interfaces/afni/utils.py Outdated Show resolved Hide resolved
nipype/interfaces/afni/preprocess.py Show resolved Hide resolved
nipype/interfaces/afni/preprocess.py Outdated Show resolved Hide resolved
Co-Authored-By: Chris Markiewicz <[email protected]>
Co-Authored-By: Chris Markiewicz <[email protected]>
Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

nipype/interfaces/afni/preprocess.py Outdated Show resolved Hide resolved
nipype/interfaces/afni/preprocess.py Show resolved Hide resolved
Co-Authored-By: Chris Markiewicz <[email protected]>
doc/interfaces.rst Outdated Show resolved Hide resolved
In particular, after allowing printing the inheritance of interfaces,
links for ZZZCommandBase interfaces were broken, as originally filtered
out and not built. Now they are being built and we are back to 5
warnings.
@oesteban
Copy link
Contributor Author

oesteban commented Jan 2, 2020

Okay, I've made sure that 1) this builds in readthedocs; 2) latex build also works; 3) tests are passing. I believe I have either addressed or opened new issues pointed out during code review and I have addressed some of the codacy problems.

@oesteban oesteban merged commit 66cd92f into nipy:master Jan 2, 2020
@oesteban oesteban deleted the doc/sphinxext-interfaces branch January 2, 2020 02:59
oesteban added a commit to oesteban/nipype that referenced this pull request Jan 2, 2020
Bringing CircleCI back to green after nipy#3124, nipy#3131, and nipy#3132.
@effigies effigies added this to the 1.4.1 milestone Jan 6, 2020
effigies pushed a commit to effigies/nipype that referenced this pull request Jan 6, 2020
Bringing CircleCI back to green after nipy#3124, nipy#3131, and nipy#3132.
@effigies effigies mentioned this pull request Jan 6, 2020
1 task
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