Skip to content

ENH: Allow afni.MaskTool to take multiple input files #2886

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

Closed
wants to merge 0 commits into from

Conversation

hstojic
Copy link
Contributor

@hstojic hstojic commented Feb 23, 2019

Summary

Fixes #2874.

List of changes proposed in this PR (pull-request)

Made a change to the inputs of MaskTool class in AFNI interface. Used InputMultiObject as suggested, tested it out and now it works fine with multiple input images as well as far as I can tell.

Acknowledgment

  • (Mandatory) I acknowledge that this contribution will be available under the Apache 2 license.

@codecov-io
Copy link

Codecov Report

Merging #2886 into master will increase coverage by 0.22%.
The diff coverage is 33.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2886      +/-   ##
==========================================
+ Coverage   67.49%   67.72%   +0.22%     
==========================================
  Files         343      343              
  Lines       43595    44115     +520     
  Branches     5424     5606     +182     
==========================================
+ Hits        29426    29877     +451     
- Misses      13465    13529      +64     
- Partials      704      709       +5
Flag Coverage Δ
#smoketests 50.48% <20%> (ø) ⬆️
#unittests 65.2% <16.66%> (+0.28%) ⬆️
Impacted Files Coverage Δ
nipype/interfaces/afni/utils.py 81.89% <100%> (ø) ⬆️
nipype/algorithms/modelgen.py 62.89% <20%> (-0.47%) ⬇️
nipype/algorithms/confounds.py 67.64% <0%> (+1.29%) ⬆️
nipype/interfaces/base/core.py 88.68% <0%> (+1.79%) ⬆️
nipype/interfaces/ants/segmentation.py 76.82% <0%> (+3.09%) ⬆️
nipype/pipeline/plugins/legacymultiproc.py 65% <0%> (+3.5%) ⬆️

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 efb4645...f1be111. Read the comment docs.

Copy link
Member

@effigies effigies left a comment

Choose a reason for hiding this comment

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

Cool, thanks. If you can clean this up to specifically target the AFNI fix and fix the minor metadata issue, this can make it in for the release tomorrow.

in_file = File(
desc='input file or files to 3dmask_tool',
in_file = InputMultiPath(
File(desc='input file or files to 3dmask_tool', exists=True),
Copy link
Member

Choose a reason for hiding this comment

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

desc should be an argument to InputMultiPath, not File, in this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, I'll correct that. Though, I simply copied a template from elsewhere in the file, e.g. same metadata issue can be found in MergeInputSpec and ZcatInputSpec class.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, not everything is consistent or correct.

if info.orth[cid] == 'Yes':
sessinfo[i]['cond'][cid]['orth'] = 1
elif info.orth[cid] == 'No':
sessinfo[i]['cond'][cid]['orth'] = 0
Copy link
Member

Choose a reason for hiding this comment

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

Can you rebase or revert the orth changes out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah, silly, I checked out a new branch from another patch :|

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oops, I did a hard reset to SHA before the orth changes and then force-push to update the remote branch, but I didnt realize that one will close the pull request. I committed the AFNI changes again, but they dont appear here anymore.

Shall I simply do another pull request?

@effigies effigies changed the title Patch afni utils mask tool ENH: Allow afni.MaskTool to take multiple input files Feb 24, 2019
@effigies effigies added this to the 1.1.9 milestone Feb 24, 2019
@hstojic hstojic closed this Feb 25, 2019
@hstojic hstojic force-pushed the patch-AFNI-utils-MaskTool branch from f1be111 to 98beb0a Compare February 25, 2019 07:18
@effigies
Copy link
Member

Feel free to open a new PR, if that's easiest.

@hstojic
Copy link
Contributor Author

hstojic commented Feb 25, 2019

Ok, opened a new PR, at #2892

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.

3 participants