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] Fixed minor typos, grammar errors and moved imports up in all examples #2151

Closed
wants to merge 25 commits into from

Conversation

ShrishtiHore
Copy link
Contributor

This PR is based on the previous PR's and discussions we had. I have moved the imports of all examples at the top of the code. I also tried fixing as many typos as I could.

It would be great if @skoudoro and @arokem please review it and give your feedback.

There are 4 files I didn't make any changes to because they were format(pattern) based examples with multiple samples in one source code. They are tracking_introduction.py, workflow_creation.py, segment_clustering_feature.py and segment_clustering_metric.py

Please check it out and tell any corrections needed.

@pep8speaks
Copy link

pep8speaks commented Apr 26, 2020

Hello @ShrishtiHore, Thank you for updating !

Line 27:1: E402 module level import not at top of file
Line 28:1: E402 module level import not at top of file
Line 29:1: E402 module level import not at top of file
Line 30:1: E402 module level import not at top of file

Line 16:1: E402 module level import not at top of file

Line 87:1: E402 module level import not at top of file
Line 88:1: E402 module level import not at top of file
Line 89:1: E402 module level import not at top of file
Line 90:1: E402 module level import not at top of file
Line 91:1: E402 module level import not at top of file
Line 92:1: E402 module level import not at top of file
Line 93:1: E402 module level import not at top of file
Line 94:1: E402 module level import not at top of file

Line 42:1: E402 module level import not at top of file

Line 75:1: E402 module level import not at top of file
Line 76:1: E402 module level import not at top of file
Line 77:1: E402 module level import not at top of file
Line 78:1: E402 module level import not at top of file
Line 104:52: W291 trailing whitespace

Line 110:81: E501 line too long (82 > 80 characters)

Line 122:81: E501 line too long (85 > 80 characters)

Comment last updated at 2020-04-28 08:00:47 UTC

@ShrishtiHore ShrishtiHore changed the title [DOC] Fixed minor typos, grammar errors and moved imports up [DOC] Fixed minor typos, grammar errors and moved imports up in all examples Apr 26, 2020
@codecov
Copy link

codecov bot commented Apr 28, 2020

Codecov Report

Merging #2151 into master will increase coverage by 0.04%.
The diff coverage is 91.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2151      +/-   ##
==========================================
+ Coverage   91.24%   91.29%   +0.04%     
==========================================
  Files         251      251              
  Lines       32228    32407     +179     
  Branches     3403     3416      +13     
==========================================
+ Hits        29407    29586     +179     
+ Misses       2084     2083       -1     
- Partials      737      738       +1     
Impacted Files Coverage Δ
dipy/io/stateful_tractogram.py 71.91% <84.61%> (+3.06%) ⬆️
dipy/io/tests/test_stateful_tractogram.py 93.20% <95.79%> (+0.77%) ⬆️
dipy/io/utils.py 86.41% <0.00%> (+7.06%) ⬆️

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.

Hi @ShrishtiHore,

Thank you for doing this.

I had a quick look at this PR and it seems there is a lot of errors due to your rebase/merge so I did not go until the end. I can see some duplication, some lines removed when they should be there, etc....

Despite these problems, I tried to generate the documentation with your branch and it is failing despite a few corrections.

We have to be really careful with the documentation. As you know, I am already working on it (#1990) by cleaning, reshaping and running all tutorials individually. It takes 1h30 to generate the whole documentation on my laptop.

So I propose you just close this PR, review mine when I will push my update. Also, you can fork my branch from #1990 and can create a PR over it on my repo.

Let me know if you have any questions.

@@ -108,7 +109,6 @@ def run(self, input_files, out_dir='', out_file='processed.nii.gz'):
file located in ``bin``.
"""

from dipy.workflows.flow_runner import run_flow
Copy link
Member

Choose a reason for hiding this comment

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

You should keep this here because if you read the tutorial, the import are in 2 different files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay thanks a lot.


from dipy.data import get_fnames
from dipy.io.image import load_nifti, save_nifti
from dipy.segment.mask import median_otsu
Copy link
Member

Choose a reason for hiding this comment

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

the block above appears twice and is duplicated with the import on the top

Copy link
Contributor Author

Choose a reason for hiding this comment

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

okay got it.

from dipy.data import get_fnames
from dipy.io.image import load_nifti, save_nifti
from dipy.segment.mask import median_otsu
from dipy.core.histeq import histeq
Copy link
Member

Choose a reason for hiding this comment

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

Why this one is not on the top ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry made a mistake in there.

@ShrishtiHore
Copy link
Contributor Author

As @skoudoro suggested I am closing this PR and I will review the #1990 PR and make some changes in same branches that he is working on. Thank You for your reply @skoudoro

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

3 participants