[ENH] Add emc-related helper functions - images

[dPys]

The helper functions are included in a new module utils/images.py, which also includes relocated functions that were previously in included in interfaces/images.py to keep things organized.

*Will require some brief unit tests.

[pull-assistant[bot]]

Best reviewed: commit by commit

---

Optimal code review plan (2 warnings)

[ENH] Add a series of general-purpose and emc-related image-handling h...

dmriprep/utils/images.py 48% changes removed in Add error-handling f...

     Update dmriprep/interfaces/images.py

     Update dmriprep/interfaces/images.py

     Update dmriprep/utils/images.py

     Update dmriprep/utils/images.py

     Update dmriprep/utils/images.py

     Update dmriprep/utils/images.py

     Update dmriprep/interfaces/images.py

     Update dmriprep/utils/images.py

     Update dmriprep/utils/images.py

     [ENH] Add docstring and first half of unit tests

     Address Sider complaint

     [DOC] Add doctests for all functions using HARDI data

     Merge branch 'master' into miscellaneous_emc_helper_functions_images

     Remove unused Pathlib import

     Add extra blank line to make sidecar happy

     Fix whitespace (again)

     Update dmriprep/utils/images.py

[ENH] Add error handling to

dmriprep/interfaces/images.py 50% changes removed in Implements summarize...

     omit prune_b0s helper function, save for later PR with signal predicti...

     Implements summarize_images -- a function to merge the functionality o...

     Implements summarize_images -- a function to merge the functionality o...

     Add error-handling for save_3d_to_4d and save_4d_to_3d

Powered by Pull Assistant. Last update 3bee02d ... 7403652. Read the comment docs.

[pep8speaks]

Hello @dPys, Thank you for updating!

Cheers! There are no style issues detected in this Pull Request. :beers: To test for issues locally, pip install flake8 and then run flake8 dmriprep.

Comment last updated at 2020-03-24 19:07:28 UTC

[codecov[bot]]

Codecov Report

Merging #77 into master will increase coverage by 2.08%. The diff coverage is 71.66%.

@@            Coverage Diff             @@
##           master      #77      +/-   ##
==========================================
+ Coverage   51.64%   53.72%   +2.08%     
==========================================
  Files          21       21              
  Lines        1218     1275      +57     
  Branches      161      171      +10     
==========================================
+ Hits          629      685      +56     
+ Misses        578      570       -8     
- Partials       11       20       +9

Impacted Files	Coverage Δ
dmriprep/utils/images.py	57.77% <68.08%> (+44.44%)	:arrow_up:
dmriprep/interfaces/images.py	73.91% <84.61%> (+3.32%)	:arrow_up:

---

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 f5c9a4f...7403652. Read the comment docs.

[josephmje]

Added a bunch of mostly nitpicking comments and made some suggestions to help shorten the line lengths for your docstrings.

[dPys]

Thanks @josephmje . I've updated the PR with your review.

[dPys]

Looks good. I would improve docstrings and adding unit tests would be very much appreciated. Also left some food for thoughts regarding the nibabel manipulations - they'd better be within Nipype.

Awesome, I think I have all the feedback I need now to finish polishing the docstring, and include some doctests.

[dPys]

@josephmje -- I went ahead on the latest commit and modified the out_path behavior, but now I see that you have been tackling this in #81 . Would you mind looking at the most recent commit here to see if this is what we want? if so, we can figure out whether we want to indeed just incorporate those changes in #81 or perhaps just keep them here since they are relatively minor tweaks that also now include docstring.

[josephmje]

@josephmje -- I went ahead on the latest commit and modified the out_path behavior, but now I see that you have been tackling this in #81 . Would you mind looking at the most recent commit here to see if this is what we want? if so, we can figure out whether we want to indeed just incorporate those changes in #81 or perhaps just keep them here since they are relatively minor tweaks that also now include docstring.

Thanks @dPys ! The docstrings look good. But I don't think the else statement will work if out_path is not defined. My PR sets the output path in the interface. Awaiting the CircleCI tests to see if things are working. Are you able to commit to my branch? It would be good to add your docstrings.

[dPys]

@josephmje -- I went ahead on the latest commit and modified the out_path behavior, but now I see that you have been tackling this in #81 . Would you mind looking at the most recent commit here to see if this is what we want? if so, we can figure out whether we want to indeed just incorporate those changes in #81 or perhaps just keep them here since they are relatively minor tweaks that also now include docstring.

Thanks @dPys ! The docstrings look good. But I don't think the else statement will work if out_path is not defined. My PR sets the output path in the interface. Awaiting the CircleCI tests to see if things are working. Are you able to commit to my branch? It would be good to add your docstrings.

Yeah the way you have it in #81 looks good!I plan to work on this tomorrow before we meet so can try to commit my changes, which include docstring and doc-tests, tomorrow morning

[dPys]

@josephmje @oesteban -- I've added all doctests for this and rebased the PR to the changes already merged in #81, which I noticed was already closed. Folks may want to quickly skim, but assuming all tests pass, this should be okay to merge?