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

Follow PEP8 in reconst (part 2) #1212

Merged
merged 11 commits into from Apr 13, 2017

Conversation

Projects
None yet
4 participants
@aaossa
Contributor

aaossa commented Apr 3, 2017

Using pycodestyle fixed every error, except for:

  • dsi.py: The long lines are TeX docstrings. Variable name I is not a problem
dsi.py:29:80: E501 line too long (116 > 79 characters)
dsi.py:245:80: E501 line too long (167 > 79 characters)
dsi.py:482:9: E741 ambiguous variable name 'I'
dsi.py:508:80: E501 line too long (174 > 79 characters)
dsi.py:509:80: E501 line too long (163 > 79 characters)
dsi.py:510:80: E501 line too long (166 > 79 characters)
  • dti.py: Line contains an FTP url
dti.py:1103:80: E501 line too long (94 > 79 characters)
  • mapmri.py: Variable names I or l are not a problem
mapmri.py:502:13: E741 ambiguous variable name 'I'
mapmri.py:504:17: E741 ambiguous variable name 'I'
mapmri.py:533:9: E741 ambiguous variable name 'I'
mapmri.py:536:13: E741 ambiguous variable name 'I'
mapmri.py:573:25: E741 ambiguous variable name 'l'
mapmri.py:624:21: E741 ambiguous variable name 'l'
mapmri.py:1331:13: E741 ambiguous variable name 'l'
mapmri.py:1383:13: E741 ambiguous variable name 'l'
mapmri.py:1406:13: E741 ambiguous variable name 'l'
mapmri.py:1449:13: E741 ambiguous variable name 'l'
mapmri.py:1501:13: E741 ambiguous variable name 'l'
mapmri.py:1526:13: E741 ambiguous variable name 'l'
mapmri.py:1584:13: E741 ambiguous variable name 'l'
mapmri.py:1639:13: E741 ambiguous variable name 'l'
mapmri.py:1647:41: E741 ambiguous variable name 'l'
mapmri.py:1686:17: E741 ambiguous variable name 'l'

This is the second PR that replaces #1207 (See #881 (comment))

@aaossa aaossa referenced this pull request Apr 3, 2017

Closed

PEP8 in reconst #881

@codecov-io

This comment has been minimized.

codecov-io commented Apr 3, 2017

Codecov Report

Merging #1212 into master will increase coverage by 0.03%.
The diff coverage is 36.84%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1212      +/-   ##
==========================================
+ Coverage   85.87%   85.91%   +0.03%     
==========================================
  Files         221      221              
  Lines       27121    27247     +126     
  Branches     2776     2782       +6     
==========================================
+ Hits        23290    23408     +118     
- Misses       3148     3154       +6     
- Partials      683      685       +2
Impacted Files Coverage Δ
dipy/reconst/dti.py 96.51% <ø> (ø) ⬆️
dipy/reconst/odf.py 92.59% <ø> (ø) ⬆️
dipy/reconst/interpolate.py 92.85% <ø> (ø) ⬆️
dipy/reconst/mapmri.py 90.47% <ø> (ø) ⬆️
dipy/reconst/fwdti.py 94.33% <100%> (ø) ⬆️
dipy/reconst/dsi.py 80.21% <100%> (ø) ⬆️
dipy/reconst/gqi.py 54.54% <29.41%> (ø) ⬆️
dipy/viz/tests/test_ui.py 85.79% <0%> (-1.79%) ⬇️
dipy/denoise/non_local_means.py 100% <0%> (ø) ⬆️
dipy/denoise/tests/test_non_local_means.py 96.72% <0%> (+0.56%) ⬆️
... and 1 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 857ca39...dfbd520. Read the comment docs.

@coveralls

This comment has been minimized.

coveralls commented Apr 3, 2017

Coverage Status

Coverage remained the same at 88.383% when pulling fe5c7f3 on aaossa:master into 520b724 on nipy:master.

@aaossa

This comment has been minimized.

Contributor

aaossa commented Apr 6, 2017

Hey @arokem can you please review this PR? Thanks! Maybe the weekend I'll be able to makea big PR with the others reconst files

@arokem

Looks good. Just one question there.

#print 'tom >>>> ',t0,t1,t2,npa
npa = np.sqrt((psi0 - psi1) ** 2 +
(psi1 - psi2) ** 2 +
(psi2 - psi0) ** 2) / \

This comment has been minimized.

@arokem

arokem Apr 6, 2017

Member

Is the \ really necessary here?

This comment has been minimized.

@aaossa

aaossa Apr 6, 2017

Contributor

It seems like it's not necessary. Do you want me to delete it?

This comment has been minimized.

@arokem

arokem Apr 6, 2017

Member

yes, please.

In general, wherever possible, I personally prefer that wrapping to more than one line be done using parentheses, rather than \. So I prefer:


    a = (b + c + d + e + 
         f + g)

rather than:


    a = b + c + d + e + \
        f + g

aaossa added some commits Apr 7, 2017

Update code in dipy/reconst (PEP8)
Using `pycodestyle` output, the file `dipy/reconst/dsi.py` was
updated to pass `pycodestyle` check

Signed-off-by: Antonio Ossa <aaossa@uc.cl>
Update code in dipy/reconst (PEP8)
Using `pycodestyle` output, the file `dipy/reconst/dti.py` was
updated to pass `pycodestyle` check

Signed-off-by: Antonio Ossa <aaossa@uc.cl>
Update code in dipy/reconst (PEP8)
Using `pycodestyle` output, the file `dipy/reconst/fwdti.py` was
updated to pass `pycodestyle` check

Signed-off-by: Antonio Ossa <aaossa@uc.cl>
Update code in dipy/reconst (PEP8)
Using `pycodestyle` output, the file `dipy/reconst/gqi.py` was
updated to pass `pycodestyle` check

Signed-off-by: Antonio Ossa <aaossa@uc.cl>
Update code in dipy/reconst (PEP8)
Using `pycodestyle` output, the file `dipy/reconst/interpolate.py` was
updated to pass `pycodestyle` check

Signed-off-by: Antonio Ossa <aaossa@uc.cl>
Update code in dipy/reconst (PEP8)
Using `pycodestyle` output, the file `dipy/reconst/mapmri.py` was
updated to pass `pycodestyle` check

Signed-off-by: Antonio Ossa <aaossa@uc.cl>
Update code in dipy/reconst (PEP8)
Using `pycodestyle` output, the file `dipy/reconst/odf.py` was
updated to pass `pycodestyle` check

Signed-off-by: Antonio Ossa <aaossa@uc.cl>
@coveralls

This comment has been minimized.

coveralls commented Apr 7, 2017

Coverage Status

Coverage remained the same at 88.383% when pulling 60fae47 on aaossa:master into 857ca39 on nipy:master.

@aaossa

This comment has been minimized.

Contributor

aaossa commented Apr 7, 2017

Added the requested change here and here

rtop = P(0)
Parameters
----------
normalized : boolean
default true, normalize the propagator by its sum in order to obtain a pdf
default true, normalize the propagator by its sum in order

This comment has been minimized.

@arokem

arokem Apr 10, 2017

Member

While we're here. Let's correct this one.

Should be:

Whether to normalize the propagator by its sum in order to obtain a pdf. Default: True. 
Parameters
----------
normalized : boolean
default true, normalize the propagator by its sum in order to obtain a pdf
default true, normalize the propagator by its sum in order
to obtain a pdf

This comment has been minimized.

@arokem

arokem Apr 10, 2017

Member

Same here.

@arokem

This comment has been minimized.

Member

arokem commented Apr 10, 2017

Just a couple more requests for changes.

Add default value to docstrings
Signed-off-by: Antonio Ossa <aaossa@uc.cl>
@aaossa

This comment has been minimized.

Contributor

aaossa commented Apr 10, 2017

Requested changes added 👌 Waiting for Travis 🤖

@coveralls

This comment has been minimized.

coveralls commented Apr 10, 2017

Coverage Status

Coverage increased (+0.03%) to 88.415% when pulling 653ab13 on aaossa:master into 857ca39 on nipy:master.

@coveralls

This comment has been minimized.

coveralls commented Apr 11, 2017

Coverage Status

Coverage increased (+0.03%) to 88.415% when pulling dfbd520 on aaossa:master into 857ca39 on nipy:master.

@arokem

This comment has been minimized.

Member

arokem commented Apr 11, 2017

Looks ready to go, from my point of view. Does anyone else want to take a look?

@arokem arokem merged commit 0297078 into nipy:master Apr 13, 2017

3 of 4 checks passed

codecov/patch 36.84% of diff hit (target 85.87%)
Details
codecov/project 85.91% (+0.03%) compared to 857ca39
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 88.415%
Details

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

Merge pull request nipy#1212 from aaossa/master
Follow PEP8 in reconst (part 2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment