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: Fix typos and formatting in .rst files and Python examples. #1314

Conversation

jhlegarreta
Copy link
Contributor

Fix typos and formatting in .rst files and Python examples'
documentations:

  • Fix typos in files.
  • Replace tabs for two white spaces.
  • Fix errors in citation and DIPY internal cross-refs.
  • Add missing cross-refs (e.g. Aganj MRM 2010).
  • Add links where necessary (ISBI HARDI Contest 2013 and FreeSurfer).
  • Fix LaTeX math formatting errors.
  • Make the example/DIPY code object markdown consistent (inverted commas).
  • Capitalize the acronym long names' first/corresponding letters (e.g.
    Constrained Spherical Deconvolution).
  • Capitalize acronyms (e.g. FA).
  • Make the writing of terms consistent in terms of uppercase/lowercase
    (e.g. b0 vs. B0; cospus callosum vs. Corpus Callosum, etc.).
  • Use the [NameYear] convention for citations.
  • Place all citations under a 'References' section, and use the subsection
    markdown for the section.
  • Use inverted commas consistently to reference code objects.
  • Make the b-value units consistent across files (s/mm^2).
  • Create and include missing figures for 'reconst_fwdti.py'.
  • Write DIPY instead of any of its variants in the documentation.
  • Link the first appearance of dipy in every file (use dipy_).

@jhlegarreta
Copy link
Contributor Author

jhlegarreta commented Aug 1, 2017

This PR addresses the issues in issue #1305.

Folks, sorry for the delay, but have been working on this only intermittently.

Thanks to all of you who made comments.

I think I've covered all I had written down in my notes. Surely I did not cover all files, nor all markdown (i.e. inverted commas for DIPY objects), so please feel free to request a revision to the PR.

I guess we'll be able to see whether the cross-refs work/have been fixed properly (e.g. cross-refs between files) once this gets merged.

I'll work on the the guidelines on a separate topic.

@Garyfallidis I tried to use DIPY systematically in the docs. I linked its first appearance with dipy_. Have also encountered dipy_. They both seemed to be linked correctly. Please, let me know which is preferred.

A few issues still remain:

However, I guess that is the way the Sphinx guys thought it should work; when clicking on the number we can go back to the location of the reference.

I guess this may be cumbersome when the same reference is used more than, say, three times in the same file.

  • In the restore_dti.py example the third reference not used: should we remove it? Have gone through the doc, but haven't found the missing location. Anyone knowing where it fits?

  • In the snr_in_cc.py file which is the right link for this?
    (see the DTI example for further explanations).

  • Is this file linked or listed anywhere?

simulate_dki.py

Should I include it in the Simulation section of examples_index.rst?

  • In the fiber_to_bundle_coherence.py example Fig. 1 is not labeled, so the first sentence is missing it.

  • The figures for the reconst_fwdti.py example are not being rendered; they are produced correctly when running the example, and have seen that the /tools/make_examples.py would compile and produce all figures to put them in /doc/examples_built/_static, but that does not seem to be happening for the above figures. The file at issue is listed in examples_index.rst, so I'm not seeing the problem. Anyone?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 85.405% when pulling c7f6c8b on jhlegarreta:FixRSTFilesAndPythonExamplesTyposAndFormatting into 0c10a43 on nipy:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 85.405% when pulling c7f6c8b on jhlegarreta:FixRSTFilesAndPythonExamplesTyposAndFormatting into 0c10a43 on nipy:master.

@codecov-io
Copy link

codecov-io commented Aug 1, 2017

Codecov Report

Merging #1314 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1314   +/-   ##
=======================================
  Coverage   87.02%   87.02%           
=======================================
  Files         228      228           
  Lines       28841    28841           
  Branches     3100     3100           
=======================================
  Hits        25099    25099           
  Misses       3037     3037           
  Partials      705      705

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 0013f57...1673bb7. Read the comment docs.

@skoudoro
Copy link
Member

skoudoro commented Aug 2, 2017

Awesome ! Thanks again ! I will review and answer yours questions as soon as possible

@jhlegarreta jhlegarreta force-pushed the FixRSTFilesAndPythonExamplesTyposAndFormatting branch from c7f6c8b to f0261bd Compare August 8, 2017 21:28
@coveralls
Copy link

Coverage Status

Coverage remained the same at 85.329% when pulling f0261bd on jhlegarreta:FixRSTFilesAndPythonExamplesTyposAndFormatting into 5595842 on nipy:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 85.329% when pulling f0261bd on jhlegarreta:FixRSTFilesAndPythonExamplesTyposAndFormatting into 5595842 on nipy:master.

@skoudoro
Copy link
Member

skoudoro commented Aug 9, 2017

In the restore_dti.py example the third reference not used: should we remove it? Have gone through the doc, but haven't found the missing location. Anyone knowing where it fits?

it fits at the end of this sentence : using the RESTORE estimate and the WLS estimate

In the snr_in_cc.py file which is the right link for this?

I do not understand what you mean here

Should I include it in the Simulation section of examples_index.rst?

yes, thanks.

In the fiber_to_bundle_coherence.py example Fig. 1 is not labeled, so the first sentence is missing it.

It seems that all description below the figures are not labeled so you can define them.

The figures for the reconst_fwdti.py example are not being rendered; ....

I think, we should create a specific issue for this one. this deserve a new PR like #1301

@jhlegarreta jhlegarreta force-pushed the FixRSTFilesAndPythonExamplesTyposAndFormatting branch from f0261bd to 8c7b3d3 Compare August 9, 2017 15:23
@jhlegarreta
Copy link
Contributor Author

jhlegarreta commented Aug 9, 2017

@skoudoro thanks for the review ! Commit 8c7b3d3 addresses the comments. I've opened the issue #1317 to track the reconst_fwdti.py example figures not being rendered.

The figures should now be labeled and referenced in fiber_to_bundle_coherence.py. However, if the entire caption is set as the link text, it will be way too long for the first figure. Any thoughts?

In the last figure of the same file, I also added the :math: syntax in an attempt to render in math syntax the threshold. Not sure whether that will work.

By,
In the snr_in_cc.py file which is the right link for this? (see the DTI example for further explanations).

I meant that the snr_in_cc.py file contains the mentioned sentence, without actually linking to any specific example, and I was wondering which file it should be linked to.

@skoudoro
Copy link
Member

skoudoro commented Aug 9, 2017

oh ok, I see, it should be linked to this example (example_reconst_dti.py)

In the last figure of the same file, I also added the :math: syntax in an attempt to render in math syntax the threshold. Not sure whether that will work.

I'm generating the whole documentation with your change. I will let you know.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 85.329% when pulling 8c7b3d3 on jhlegarreta:FixRSTFilesAndPythonExamplesTyposAndFormatting into 5595842 on nipy:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 85.329% when pulling 8c7b3d3 on jhlegarreta:FixRSTFilesAndPythonExamplesTyposAndFormatting into 5595842 on nipy:master.

@jhlegarreta jhlegarreta force-pushed the FixRSTFilesAndPythonExamplesTyposAndFormatting branch from 8c7b3d3 to 52cf453 Compare August 10, 2017 05:42
@jhlegarreta
Copy link
Contributor Author

@skoudoro Done for the link of reconst_dti.py when cross-ref'ing in snr_in_cc.py.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 85.329% when pulling 52cf453 on jhlegarreta:FixRSTFilesAndPythonExamplesTyposAndFormatting into 5595842 on nipy:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 85.329% when pulling 52cf453 on jhlegarreta:FixRSTFilesAndPythonExamplesTyposAndFormatting into 5595842 on nipy:master.

@jhlegarreta
Copy link
Contributor Author

@skoudoro Thanks for having a look to the doc generation ! Please, pay attention to some links; I'm not sure whether Sphinx allows to break the link text or link text vs. link across lines, which I've done.

Also, across the website some paths are written as <./doc/devel> while others are simply formatted as doc/devel. Just a minor detail, but for the sake of consistency, which one should we encourage?

Copy link
Member

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Thanks again @jhlegarreta. In general, it is ok but there is some problem with dipy_ link in many examples. Some remarks:

  • 'out_' : strange rendering on this file
  • .. admonition rendering problem on this file
  • wrong rendering for dipy.tracking.life here
  • in FAQ, numpy and cython links are not present
  • DIPY is not modified on these files : release*.rst, index.rst, old_news.rst, old_highlight.rst

@@ -75,7 +75,7 @@ Release checklist
``README`` in the root directory, maybe with ``vim`` ``diffthis`` command.
Check all the links are still valid.

* Check all the dipy builds are green on the `nipy buildbot`_
* Check all the DIPY builds are green on the `nipy buildbot`_

* If you have travis-ci_ building set up you might want to push the code in its
Copy link
Member

Choose a reason for hiding this comment

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

missing the link to travis CI website

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@skoudoro Thanks for giving it a try!

New patch set 412e9fb:

As for the http://nipy.org/dipy/reference_cmd/CombinedWorkflow.html (combined_workflow_creation.py) file's strange rendering, I cannot say much; I've gone through it but did not find any error.

As for the files with .. admonition::`` rendering problems (combined_workflow_cration.py, gradients_spheres.py`), I did not find a plausible explanation.

As for the wrong rendering for dipy.tracking.life, I changed to double inverted commas.

As for the links in FAQ, I think I fixed the links for numpy and cython.

Changed the DIPY case in the mentioned files. Had not changed the release notes because I thought at the time the preferred case was the way it was. But changed in the titles now.

Added travis-ci to link-names.inc. See whether this solves the issue.

Eleftherios if you do.

At the moment, the useful dipy binary build testers are:
At the moment, the useful DIPY binary build testers are:

* http://nipy.bic.berkeley.edu/builders/dipy-bdist32-26
Copy link
Member

Choose a reason for hiding this comment

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

this one does not exist anymore, can you replace it by http://nipy.bic.berkeley.edu/builders/dipy-bdist32-35
and can you add :
http://nipy.bic.berkeley.edu/builders/dipy-bdist64-27
http://nipy.bic.berkeley.edu/builders/dipy-bdist64-35

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed in 412e9fb.

@@ -3,8 +3,8 @@
Brain segmentation with median_otsu
===================================

We show how to extract brain information and mask from a b0 image using dipy's
segment.mask module.
We show how to extract brain information and mask from a b0 image using dipy_'s
Copy link
Member

Choose a reason for hiding this comment

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

link does not work. it refers to #id2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd dare to say that the incorrect links to DIPY (dipy_) were due to the missing include links_names.inc. I've included it in the reported files. See whether this solves the issue.

@@ -3,13 +3,13 @@
Creating a new combined workflow.
============================================================

A combined workflow is a series of dipy workflows organized together in a way
that the output of a workflow serves as input for the next one.
A ``CombinedWorkflow`` is a series of dipy_ workflows organized together in a
Copy link
Member

Choose a reason for hiding this comment

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

dipy_ link does not work. it refers to #id1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

(150 orientations, b=2000s/mm^2) which is one of the standard example datasets
in DIPY.
(150 orientations, b=2000 $s/mm^2$) which is one of the standard example
datasets in dipy_.
Copy link
Member

Choose a reason for hiding this comment

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

same as above, this link does not work

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

@@ -3,19 +3,19 @@
Visualize surfaces
==================

Here is a simple tutorial that shows how to visualize surfaces using DIPY. It
also shows how to load/save, get/set and update vtkPolyData and show
Here is a simple tutorial that shows how to visualize surfaces using dipy_. It
Copy link
Member

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

@@ -3,16 +3,18 @@
Creating a new workflow.
============================================================

A workflow is a series of dipy operations with fixed inputs and outputs
A workflow is a series of dipy_ operations with fixed inputs and outputs
Copy link
Member

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

that is callable via command line or another interface.

For example, after installing dipy, you can call anywhere from your command line:
For example, after installing dipy_, you can call anywhere from your command
Copy link
Member

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

@@ -197,6 +197,7 @@ Simulations

- :ref:`example_simulate_multi_tensor`
- :ref:`example_reconst_dsid`
- :ref:`simulate_dki`
Copy link
Member

Choose a reason for hiding this comment

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

should be example_simulate_dki instead of simulate_dki

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Fixed in 412e9fb.

@@ -6,7 +6,7 @@

Mission of Statement

The purpose of dipy is to make it **easier to do better diffusion MR imaging research**. Following up with the nipy mission statement we aim to build software that is
The purpose of dipy_ is to make it **easier to do better diffusion MR imaging research**. Following up with the nipy mission statement we aim to build software that is
Copy link
Member

Choose a reason for hiding this comment

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

same as above

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same as above.

@jhlegarreta jhlegarreta force-pushed the FixRSTFilesAndPythonExamplesTyposAndFormatting branch from 52cf453 to 412e9fb Compare August 11, 2017 17:03
@coveralls
Copy link

coveralls commented Aug 11, 2017

Coverage Status

Coverage decreased (-0.002%) to 85.327% when pulling 412e9fb on jhlegarreta:FixRSTFilesAndPythonExamplesTyposAndFormatting into 5595842 on nipy:master.

Copy link
Member

@skoudoro skoudoro left a comment

Choose a reason for hiding this comment

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

Thank you for this correction, All links work well now. After generating the documentation, Can you correct this following point :


lambdaN and lambdaL are the radial and angular regularization constants,
lambdaN`` and ``lambdaL`` are the radial and angular regularization constants,
Copy link
Member

Choose a reason for hiding this comment

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

missing `` before lambdaN

"""

evecs_img = nib.Nifti1Image(tenfit.evecs.astype(np.float32), img.affine)
nib.save(evecs_img, 'tensor_evecs.nii.gz')

"""
Other tensor statistics can be calculated from the `tenfit` object. For example,
Other tensor statistics can be calculated from the `tenfit``` object. For example,
Copy link
Member

Choose a reason for hiding this comment

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

tenfit rendering error. missing ` at begin

diffusivity. One is by calling the `mean_diffusivity` module function on the
eigen-values of the TensorFit class instance:
measures. In DIPY, there are two equivalent ways to calculate the mean
diffusivity. One is by calling the `mean_diffusivity``` module function on the
Copy link
Member

Choose a reason for hiding this comment

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

mean_diffusivity rendering error. missing ` at begin

@@ -24,25 +27,25 @@

"""
For the simulation we will need a GradientTable with the b-values and
b-vectors. Here we use the GradientTable of the sample Dipy dataset
'small_64D'.
b-vectors. Here we use the GradientTable of the sample dipy_ dataset
Copy link
Member

Choose a reason for hiding this comment

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

the link does not work. Missing the include

@jhlegarreta jhlegarreta force-pushed the FixRSTFilesAndPythonExamplesTyposAndFormatting branch from 412e9fb to 989c78d Compare September 7, 2017 16:37
@jhlegarreta
Copy link
Contributor Author

@skoudoro Thanks. Sorry for the delay. I've been away from keyboard until a few days ago. Partially addressed your comments.

Typo of dipy.reconst.cross_validation on kfold_xval.py

I do not see where the typo is. According to http://www.sphinx-doc.org/en/stable/domains.html#cross-referencing-python-objects, the markup is fine.

And in fact, that makes me think about changing the statement about using the backquote or inverted commas inline markup for DIPY objects (see the sentence The classes, objects, and any other construct referenced from the code should be written with inverted commas, (...) in #1305).

I'd vote for using proper Sphinx cross-referencing/syntax (such as in:mod:reconst.cross_validation or its correct form; or :class:GradientTable which holds all the acquisition specific parameters (...) instead of ``GradientTable`` which holds all the acquisition specific parameters (...)).

At least for those objects that are present in the dipy code. I ignore whether the rule should also be applied to the objects only present in the example files, or if these should be fine with the inverted commas syntax.

If you do agree, I'd change the above as far as I'm able to in a subsequent patch set.

Also addressed @skoudoro's comments in #1305 concerning the captions and the path syntax.

Removed remaining tabs, and did some other minor changes for the sake of consistency.

@jhlegarreta
Copy link
Contributor Author

The failing build in Travis seems to be unrelated to the patch set. I ignore the meaning of the USE_PRE flag, but I've seen that other builds have the build marked as Allowed Failures (e.g. https://travis-ci.org/nipy/dipy/builds/214169250).

@arokem
Copy link
Contributor

arokem commented Sep 8, 2017

You are seeing #1323

@jhlegarreta
Copy link
Contributor Author

@arokem OK, thanks for clarifying this.

@jhlegarreta jhlegarreta force-pushed the FixRSTFilesAndPythonExamplesTyposAndFormatting branch from 989c78d to 5bb52b6 Compare September 8, 2017 17:31
@jhlegarreta
Copy link
Contributor Author

Rebased to fix the conflict with master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 85.333% when pulling 5bb52b6 on jhlegarreta:FixRSTFilesAndPythonExamplesTyposAndFormatting into e498e68 on nipy:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 85.333% when pulling 5bb52b6 on jhlegarreta:FixRSTFilesAndPythonExamplesTyposAndFormatting into e498e68 on nipy:master.

Fix typos and formatting in .rst files and Python examples'
documentations:
- Fix typos in files.
- Replace tabs for two white spaces.
- Fix errors in citation and DIPY internal cross-refs.
- Add missing cross-refs (e.g. Aganj MRM 2010).
- Add links where necessary (ISBI HARDI Contest 2013 and FreeSurfer).
- Fix LaTeX math formatting errors.
- Make the example/DIPY code object markdown consistent (inverted commas).
- Capitalize the acronym long names' first/corresponding letters (e.g.
  Constrained Spherical Deconvolution).
- Capitalize acronyms (e.g. FA).
- Make the writing of terms consistent in terms of uppercase/lowercase
  (e.g. b0 vs. B0; cospus callosum vs. Corpus Callosum, etc.).
- Use the [NameYear] convention for citations.
- Place all citations under a 'References' section, and use the subsection
  markdown for the section.
- Use inverted commas consistently to reference code objects.
- Make the b-value units consistent across files (s/mm^2).
- Create and include missing figures for 'reconst_fwdti.py'.
- Write DIPY instead of any of its variants in the documentation.
- Link the first appearance of dipy in every file (use dipy_).
@jhlegarreta jhlegarreta force-pushed the FixRSTFilesAndPythonExamplesTyposAndFormatting branch from 5bb52b6 to 1673bb7 Compare September 20, 2017 07:54
@skoudoro skoudoro merged commit 76d021a into dipy:master Oct 6, 2017
@skoudoro
Copy link
Member

skoudoro commented Oct 6, 2017

Thanks @jhlegarreta ! I merge this one

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018
…mplesTyposAndFormatting

DOC: Fix typos and formatting in .rst files and Python examples.
@jhlegarreta jhlegarreta deleted the FixRSTFilesAndPythonExamplesTyposAndFormatting branch October 10, 2018 02:15
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.

None yet

5 participants