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

Merged
merged 1 commit into from Oct 6, 2017

Conversation

Projects
None yet
5 participants
@jhlegarreta
Contributor

jhlegarreta commented Aug 1, 2017

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

This comment has been minimized.

Contributor

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

This comment has been minimized.

coveralls commented Aug 1, 2017

Coverage Status

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

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Aug 1, 2017

Coverage Status

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

@codecov-io

This comment has been minimized.

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

This comment has been minimized.

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 jhlegarreta:FixRSTFilesAndPythonExamplesTyposAndFormatting branch from c7f6c8b to f0261bd Aug 8, 2017

@coveralls

This comment has been minimized.

coveralls commented Aug 8, 2017

Coverage Status

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

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Aug 8, 2017

Coverage Status

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

@skoudoro

This comment has been minimized.

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 jhlegarreta:FixRSTFilesAndPythonExamplesTyposAndFormatting branch from f0261bd to 8c7b3d3 Aug 9, 2017

@jhlegarreta

This comment has been minimized.

Contributor

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

This comment has been minimized.

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

This comment has been minimized.

coveralls commented Aug 9, 2017

Coverage Status

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

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Aug 9, 2017

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 jhlegarreta:FixRSTFilesAndPythonExamplesTyposAndFormatting branch from 8c7b3d3 to 52cf453 Aug 10, 2017

@jhlegarreta

This comment has been minimized.

Contributor

jhlegarreta commented Aug 10, 2017

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

@coveralls

This comment has been minimized.

coveralls commented Aug 10, 2017

Coverage Status

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

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Aug 10, 2017

Coverage Status

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

@jhlegarreta

This comment has been minimized.

Contributor

jhlegarreta commented Aug 10, 2017

@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?

@skoudoro

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

This comment has been minimized.

@skoudoro

skoudoro Aug 10, 2017

Member

missing the link to travis CI website

This comment has been minimized.

@jhlegarreta

jhlegarreta Aug 11, 2017

Contributor

@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

This comment has been minimized.

@skoudoro

skoudoro Aug 10, 2017

Member

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

This comment has been minimized.

@jhlegarreta

jhlegarreta Aug 11, 2017

Contributor

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

This comment has been minimized.

@skoudoro

skoudoro Aug 10, 2017

Member

link does not work. it refers to #id2

This comment has been minimized.

@jhlegarreta

jhlegarreta Aug 11, 2017

Contributor

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

This comment has been minimized.

@skoudoro

skoudoro Aug 10, 2017

Member

dipy_ link does not work. it refers to #id1

This comment has been minimized.

@jhlegarreta

jhlegarreta Aug 11, 2017

Contributor

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_.

This comment has been minimized.

@skoudoro

skoudoro Aug 10, 2017

Member

same as above, this link does not work

This comment has been minimized.

@jhlegarreta

jhlegarreta Aug 11, 2017

Contributor

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

This comment has been minimized.

@skoudoro

skoudoro Aug 10, 2017

Member

same as above

This comment has been minimized.

@jhlegarreta

jhlegarreta Aug 11, 2017

Contributor

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

This comment has been minimized.

@skoudoro

skoudoro Aug 10, 2017

Member

same as above

This comment has been minimized.

@jhlegarreta

jhlegarreta Aug 11, 2017

Contributor

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

This comment has been minimized.

@skoudoro

skoudoro Aug 10, 2017

Member

same as above

This comment has been minimized.

@jhlegarreta

jhlegarreta Aug 11, 2017

Contributor

Same as above.

@@ -197,6 +197,7 @@ Simulations
- :ref:`example_simulate_multi_tensor`
- :ref:`example_reconst_dsid`
- :ref:`simulate_dki`

This comment has been minimized.

@skoudoro

skoudoro Aug 10, 2017

Member

should be example_simulate_dki instead of simulate_dki

This comment has been minimized.

@jhlegarreta

jhlegarreta Aug 11, 2017

Contributor

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

This comment has been minimized.

@skoudoro

skoudoro Aug 10, 2017

Member

same as above

This comment has been minimized.

@jhlegarreta

jhlegarreta Aug 11, 2017

Contributor

Same as above.

@jhlegarreta jhlegarreta force-pushed the jhlegarreta:FixRSTFilesAndPythonExamplesTyposAndFormatting branch from 52cf453 to 412e9fb Aug 11, 2017

@coveralls

This comment has been minimized.

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.

@skoudoro

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,

This comment has been minimized.

@skoudoro

skoudoro Aug 21, 2017

Member

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,

This comment has been minimized.

@skoudoro

skoudoro Aug 21, 2017

Member

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

This comment has been minimized.

@skoudoro

skoudoro Aug 21, 2017

Member

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

This comment has been minimized.

@skoudoro

skoudoro Aug 21, 2017

Member

the link does not work. Missing the include

@jhlegarreta jhlegarreta force-pushed the jhlegarreta:FixRSTFilesAndPythonExamplesTyposAndFormatting branch from 412e9fb to 989c78d Sep 7, 2017

@jhlegarreta

This comment has been minimized.

Contributor

jhlegarreta commented Sep 7, 2017

@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

This comment has been minimized.

Contributor

jhlegarreta commented Sep 8, 2017

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

This comment has been minimized.

Member

arokem commented Sep 8, 2017

You are seeing #1323

@jhlegarreta

This comment has been minimized.

Contributor

jhlegarreta commented Sep 8, 2017

@arokem OK, thanks for clarifying this.

@jhlegarreta jhlegarreta force-pushed the jhlegarreta:FixRSTFilesAndPythonExamplesTyposAndFormatting branch from 989c78d to 5bb52b6 Sep 8, 2017

@jhlegarreta

This comment has been minimized.

Contributor

jhlegarreta commented Sep 8, 2017

Rebased to fix the conflict with master.

@coveralls

This comment has been minimized.

coveralls commented Sep 8, 2017

Coverage Status

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

1 similar comment
@coveralls

This comment has been minimized.

coveralls commented Sep 8, 2017

Coverage Status

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

Jon Haitz Legarreta Gorroño
DOC: Fix typos and formatting in .rst files and Python examples.
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 jhlegarreta:FixRSTFilesAndPythonExamplesTyposAndFormatting branch from 5bb52b6 to 1673bb7 Sep 20, 2017

@skoudoro skoudoro merged commit 76d021a into nipy:master Oct 6, 2017

3 checks passed

codecov/patch Coverage not affected when comparing 0013f57...1673bb7
Details
codecov/project 87.02% remains the same compared to 0013f57
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@skoudoro

This comment has been minimized.

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

Merge pull request nipy#1314 from jhlegarreta/FixRSTFilesAndPythonExa…
…mplesTyposAndFormatting

DOC: Fix typos and formatting in .rst files and Python examples.

@jhlegarreta jhlegarreta deleted the jhlegarreta:FixRSTFilesAndPythonExamplesTyposAndFormatting branch Oct 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment