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

simplify SDR iterate #1470

Closed
wants to merge 0 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@Borda
Contributor

Borda commented Mar 20, 2018

simplify the _iterate function in SDRegistration to be easier to read

@Borda Borda force-pushed the Borda:master branch from c2b2ec2 to 3c3e215 Mar 20, 2018

@codecov-io

This comment has been minimized.

codecov-io commented Mar 20, 2018

Codecov Report

Merging #1470 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1470      +/-   ##
==========================================
+ Coverage    87.5%   87.51%   +<.01%     
==========================================
  Files         241      241              
  Lines       30789    30791       +2     
  Branches     3322     3321       -1     
==========================================
+ Hits        26943    26946       +3     
  Misses       3071     3071              
+ Partials      775      774       -1
Impacted Files Coverage Δ
dipy/align/imwarp.py 98.62% <100%> (+0.23%) ⬆️

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 7f7dc1e...6138503. Read the comment docs.

@Borda

This comment has been minimized.

Contributor

Borda commented Mar 20, 2018

I think that it would be nice to make some more code cleaning with a help of an automatic tool...

@Borda Borda force-pushed the Borda:master branch 2 times, most recently from a5201f4 to 23dca20 Mar 20, 2018

@skoudoro

This comment has been minimized.

Member

skoudoro commented Mar 20, 2018

Hi @Borda, Thank you really much for this!

I will get back to you ASAP concerning this big PR.

Concerning an automatic tool for checking code quality, I would like to have @Garyfallidis and @arokem opinion.

@Borda

This comment has been minimized.

Contributor

Borda commented Mar 21, 2018

Btw, should the code also be by PEP8? because I saw many incompatibilities there...

@arokem

This comment has been minimized.

Member

arokem commented Mar 21, 2018

@skoudoro

This comment has been minimized.

Member

skoudoro commented Mar 21, 2018

I agree with @arokem, it will be good if you can split this PR into 2 PRs at least: SDR simplification and code quality

@Borda Borda force-pushed the Borda:master branch from 23dca20 to 57fa3ec Mar 21, 2018

@Borda

This comment has been minimized.

Contributor

Borda commented Mar 21, 2018

@arokem @skoudoro Ok, see #1471

""" set zero displacements at the boundary
:param ndarray step:
:return ndarray:

This comment has been minimized.

@skoudoro

skoudoro Mar 22, 2018

Member

Can you update the docstring by using Numpy documentation standard

@skoudoro

Looks good to me, just need a documentation update.

Thanks @Borda!

:param current_disp_world2grid:
:param current_disp_spacing:
"""

This comment has been minimized.

@skoudoro

skoudoro Mar 22, 2018

Member

same as above

@Borda Borda force-pushed the Borda:master branch from 57fa3ec to 834b620 Mar 22, 2018

@skoudoro

This comment has been minimized.

Member

skoudoro commented Mar 22, 2018

It seems ready to go!

I will merge it tomorrow. if there is no objection.

@Borda Borda force-pushed the Borda:master branch from 6138503 to 2bb134a Mar 23, 2018

@Borda Borda closed this Mar 23, 2018

@Borda Borda force-pushed the Borda:master branch from 2bb134a to b8e01ee Mar 23, 2018

@Borda

This comment has been minimized.

Contributor

Borda commented Mar 23, 2018

now in #1475 since somehow after rebase it was automatically closed

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