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 1) #1208

Merged
merged 6 commits into from Apr 3, 2017

Conversation

Projects
None yet
5 participants
@aaossa
Contributor

aaossa commented Mar 31, 2017

Using pycodestyle fixed every error, except for:

  • csdeconv.py: The cited lines are docstrings
dipy/reconst\csdeconv.py:599:80: E501 line too long (88 > 79 characters)
dipy/reconst\csdeconv.py:601:80: E501 line too long (92 > 79 characters)

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

aaossa added some commits Mar 31, 2017

Update code in dipy/reconst (PEP8)
Using `pycodestyle` output, the file `dipy/reconst/__init__.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/base.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/cache.py` was
updated to pass `pycodestyle` check

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

@aaossa aaossa referenced this pull request Mar 31, 2017

Closed

PEP8 in reconst #881

aaossa added some commits Mar 31, 2017

Update code in dipy/reconst (PEP8)
Using `pycodestyle` output, the file `dipy/reconst/csdeconv.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/dki.py` was
updated to pass `pycodestyle` check

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

arokem approved these changes Mar 31, 2017

Looks good to me. Just one small request for change here.

int(ci - w): int(ci + w),
int(cj - w): int(cj + w),
int(ck - w): int(ck + w)
]

This comment has been minimized.

@arokem

arokem Mar 31, 2017

Member

Would you mind rewriting this as follows?

data[int(ci - w): int(ci + w),
     int(cj - w): int(cj + w),
     int(ck - w): int(ck + w)]

This comment has been minimized.

@aaossa

aaossa Mar 31, 2017

Contributor

Fixed by 78935bf 👌

@aaossa

This comment has been minimized.

Contributor

aaossa commented Mar 31, 2017

@arokem you kwow what could be causing the build to fail? I'm traying to see if my changes have some problem

@@ -275,7 +279,7 @@ def __init__(self, gtab, ratio, reg_sphere=None, sh_order=8, lambda_=1.,

no_params = ((sh_order + 1) * (sh_order + 2)) / 2

if no_params > np.sum(gtab.b0s_mask == False):
if no_params > np.sum(gtab.b0s_mask is False):

This comment has been minimized.

@aaossa

aaossa Mar 31, 2017

Contributor

Maybe this line is causing a problem. I'll try replacing it with:

bool(gtab.b0s_mask) is False

@@ -275,7 +279,7 @@ def __init__(self, gtab, ratio, reg_sphere=None, sh_order=8, lambda_=1.,

no_params = ((sh_order + 1) * (sh_order + 2)) / 2

if no_params > np.sum(gtab.b0s_mask == False):
if no_params > np.sum(gtab.b0s_mask is False):

This comment has been minimized.

@aaossa

aaossa Mar 31, 2017

Contributor

Same here (see above)

@arokem

This comment has been minimized.

Member

arokem commented Mar 31, 2017

Even better:

if no_params > np.sum(~gtab.b0s_mask):
Fix current build fail
Convert variable explicitly to bool to compare with False

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

This comment has been minimized.

Contributor

aaossa commented Apr 1, 2017

I'll try that right now, hope it finally builds! I think that the problem were caused because we're working with arrays here, I didn't check that, I'm sorry 🙉

@coveralls

This comment has been minimized.

coveralls commented Apr 1, 2017

Coverage Status

Coverage remained the same at 88.381% when pulling 7765859 on aaossa:master into 69c7093 on nipy:master.

@codecov-io

This comment has been minimized.

codecov-io commented Apr 1, 2017

Codecov Report

Merging #1208 into master will increase coverage by 0.1%.
The diff coverage is 98.11%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #1208     +/-   ##
=========================================
+ Coverage   85.87%   85.98%   +0.1%     
=========================================
  Files         221      194     -27     
  Lines       27116    26834    -282     
  Branches     2776     2771      -5     
=========================================
- Hits        23285    23072    -213     
+ Misses       3148     3099     -49     
+ Partials      683      663     -20
Impacted Files Coverage Δ
dipy/reconst/cache.py 100% <ø> (ø) ⬆️
dipy/reconst/base.py 75.88% <100%> (-13.01%) ⬇️
dipy/reconst/dki.py 97.63% <100%> (ø) ⬆️
dipy/reconst/csdeconv.py 88.52% <87.5%> (ø) ⬆️
dipy/align/metrics.py 92.84% <0%> (-3.73%) ⬇️
dipy/core/gradients.py 93.54% <0%> (-3.46%) ⬇️
dipy/reconst/peaks.py
dipy/viz/tests/__init__.py
dipy/core/tests/__init__.py
... and 28 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 69c7093...7765859. Read the comment docs.

@aaossa

This comment has been minimized.

Contributor

aaossa commented Apr 1, 2017

Thanks for your suggestion @arokem , it fixed the builds 😁 I think now it's ready to be merged

@arokem

This comment has been minimized.

Member

arokem commented Apr 1, 2017

LGTM. Anyone else want to take a look?

@arokem arokem merged commit 520b724 into nipy:master Apr 3, 2017

4 checks passed

codecov/patch 98.11% of diff hit (target 85.87%)
Details
codecov/project 85.98% (+0.1%) compared to 69c7093
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 88.381%
Details

@aaossa aaossa changed the title from Follow PEP8 in reconst to Follow PEP8 in reconst (part 1) Apr 3, 2017

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

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