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

[ENH] Add emc-related helper functions - images #77

Open
wants to merge 23 commits into
base: master
Choose a base branch
from

Conversation

dPys
Copy link
Collaborator

@dPys dPys commented Feb 11, 2020

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
Copy link

pull-assistant bot commented Feb 11, 2020

Score: 0.94

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
Copy link

pep8speaks commented Feb 11, 2020

Hello @dPys, Thank you for updating!

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

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

@dPys dPys force-pushed the miscellaneous_emc_helper_functions_images branch from a22eccf to 6c752f1 Compare February 11, 2020 23:14
@codecov
Copy link

codecov bot commented Feb 11, 2020

Codecov Report

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

Impacted file tree graph

@@            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%) ⬆️
dmriprep/interfaces/images.py 73.91% <84.61%> (+3.32%) ⬆️

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.

@dPys dPys force-pushed the miscellaneous_emc_helper_functions_images branch from 6c752f1 to 5f5ebb1 Compare February 11, 2020 23:15
@dPys dPys changed the title [ENH] Add a general-purpose and emc-related image-handling helper functions [ENH] Add general-purpose and emc-related image-handling helper functions Feb 11, 2020
@dPys dPys changed the title [ENH] Add general-purpose and emc-related image-handling helper functions [ENH] Add general-purpose and emc-related image helper functions Feb 11, 2020
@dPys dPys changed the title [ENH] Add general-purpose and emc-related image helper functions [ENH] Add emc-related image helper functions Feb 11, 2020
@dPys dPys changed the title [ENH] Add emc-related image helper functions [ENH] Add emc-related helper functions - images Feb 11, 2020
@dPys dPys requested review from oesteban and josephmje and removed request for oesteban February 12, 2020 02:48
dmriprep/interfaces/images.py Outdated Show resolved Hide resolved
dmriprep/interfaces/images.py Outdated Show resolved Hide resolved
dmriprep/interfaces/images.py Outdated Show resolved Hide resolved
dmriprep/utils/images.py Outdated Show resolved Hide resolved
dmriprep/utils/images.py Outdated Show resolved Hide resolved
dmriprep/utils/images.py Outdated Show resolved Hide resolved
dmriprep/utils/images.py Outdated Show resolved Hide resolved
dmriprep/utils/images.py Outdated Show resolved Hide resolved
dmriprep/utils/images.py Outdated Show resolved Hide resolved
dmriprep/utils/images.py Outdated Show resolved Hide resolved
@josephmje
Copy link
Collaborator

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

dmriprep/utils/images.py Outdated Show resolved Hide resolved
…helper functions in a new module utils/images.py, and relocate dangling image helper functions that were previously in interfaces/images.py into this module
@dPys dPys force-pushed the miscellaneous_emc_helper_functions_images branch from 5f5ebb1 to 3bee02d Compare February 12, 2020 19:00
@dPys
Copy link
Collaborator Author

dPys commented Feb 12, 2020

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

Copy link
Collaborator

@josephmje josephmje left a comment

Choose a reason for hiding this comment

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

Thanks @dPys . Looks good to me.

arokem added a commit to arokem/dmriprep that referenced this pull request Feb 25, 2020
Copy link
Member

@oesteban oesteban left a comment

Choose a reason for hiding this comment

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

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.

dmriprep/interfaces/images.py Outdated Show resolved Hide resolved
dmriprep/interfaces/images.py Outdated Show resolved Hide resolved
dmriprep/interfaces/images.py Outdated Show resolved Hide resolved
dmriprep/utils/images.py Outdated Show resolved Hide resolved
dmriprep/utils/images.py Outdated Show resolved Hide resolved
dmriprep/utils/images.py Outdated Show resolved Hide resolved
dmriprep/utils/images.py Outdated Show resolved Hide resolved
dmriprep/utils/images.py Outdated Show resolved Hide resolved
dmriprep/utils/images.py Outdated Show resolved Hide resolved
dmriprep/utils/images.py Outdated Show resolved Hide resolved
@dPys
Copy link
Collaborator Author

dPys commented Feb 25, 2020

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.

dmriprep/utils/images.py Outdated Show resolved Hide resolved
@dPys
Copy link
Collaborator Author

dPys commented Mar 17, 2020

@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
Copy link
Collaborator

@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.

@oesteban oesteban added this to the 0.4.0 milestone Mar 23, 2020
@dPys
Copy link
Collaborator Author

dPys commented Mar 24, 2020

@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
Copy link
Collaborator Author

dPys commented Mar 24, 2020

@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?

dmriprep/utils/images.py Outdated Show resolved Hide resolved
dmriprep/interfaces/images.py Outdated Show resolved Hide resolved
dmriprep/interfaces/images.py Outdated Show resolved Hide resolved
@dPys dPys force-pushed the miscellaneous_emc_helper_functions_images branch 2 times, most recently from 7d82850 to a70374f Compare March 24, 2020 18:09
…of median_image and average_image using a callable argument
@dPys dPys force-pushed the miscellaneous_emc_helper_functions_images branch from a70374f to 7767498 Compare March 24, 2020 18:53
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.

6 participants