Skip to content

ENH: Allow input weight for afni's volreg. #2155

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 4 commits into from

Conversation

williford
Copy link
Contributor

This PR should be discussed before accepting.

I've added the following to VolregInputSpec:

    in_weight_file = File(
        desc='File for input weighting volume',
        argstr="-weight '%s[0]'",
        exists=True)

Apparently, 3dvolreg requires the volume number for the weight. I don't know of a good way to do this with NiPype, so I make it just take the first volume (I think typically someone would only pass a single volume image). Is this sufficient?

Also there doesn't seem to be a standard way for naming the input weights. I went with a more verbose version, but feedback is welcome.

I tried to run make check-before-commit and received the error "ImportError: No module named packaging.version". I'm running the code in virtualenv with Python3, using pip to install.

@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2155      +/-   ##
==========================================
+ Coverage   72.25%   72.25%   +<.01%     
==========================================
  Files        1168     1168              
  Lines       58405    58406       +1     
  Branches     8400     8400              
==========================================
+ Hits        42201    42204       +3     
+ Misses      14870    14868       -2     
  Partials     1334     1334
Flag Coverage Δ
#smoketests 72.25% <100%> (ø) ⬆️
#unittests 69.91% <100%> (ø) ⬆️
Impacted Files Coverage Δ
nipype/interfaces/afni/preprocess.py 81.4% <100%> (+0.02%) ⬆️
nipype/interfaces/base.py 83.48% <0%> (+0.18%) ⬆️

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 32e724c...ab14a0e. Read the comment docs.

@codecov-io
Copy link

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2155      +/-   ##
==========================================
+ Coverage   72.25%   72.25%   +<.01%     
==========================================
  Files        1168     1168              
  Lines       58405    58406       +1     
  Branches     8400     8400              
==========================================
+ Hits        42201    42204       +3     
+ Misses      14870    14868       -2     
  Partials     1334     1334
Flag Coverage Δ
#smoketests 72.25% <100%> (ø) ⬆️
#unittests 69.91% <100%> (ø) ⬆️
Impacted Files Coverage Δ
nipype/interfaces/afni/preprocess.py 81.4% <100%> (+0.02%) ⬆️
nipype/interfaces/base.py 83.48% <0%> (+0.18%) ⬆️

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 32e724c...ab14a0e. Read the comment docs.

@codecov-io
Copy link

codecov-io commented Aug 17, 2017

Codecov Report

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

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2155      +/-   ##
==========================================
+ Coverage   72.25%   72.25%   +<.01%     
==========================================
  Files        1168     1168              
  Lines       58405    58406       +1     
  Branches     8400     8400              
==========================================
+ Hits        42203    42204       +1     
  Misses      14868    14868              
  Partials     1334     1334
Flag Coverage Δ
#smoketests 72.25% <100%> (ø) ⬆️
#unittests 69.91% <100%> (ø) ⬆️
Impacted Files Coverage Δ
nipype/interfaces/afni/preprocess.py 81.4% <100%> (+0.02%) ⬆️

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 32e724c...6e1a392. Read the comment docs.

@effigies
Copy link
Member

effigies commented Sep 8, 2017

One option is to use a tuple trait, which would require the user to specify the volume.

in_weight_volume = traits.Tuple(File(exists=True), traits.Int, argstr="-weight '%s[%d]'", ...)

However, using the 0th volume seems like a reasonable default, so what about the following:

in_weight_volume = traits.Either(traits.Tuple(File(exists=True), traits.Int),
                                 File(exists=True), argstr="-weight '%s[%d]'", ...)

Then, in the interface class, override _format_arg:

def _format_arg(self, name, trait_spec, value):
    if name == 'in_weight_volume' and not isinstance(value, Tuple):
        value = (value, 0)
    return super(Volreg, self)._format_arg(name, trait_spec, value)

Thus, if you pass volreg.inputs.in_weight_volume = 'functional.nii', you get -weight 'functional.nii[0]', but if you pass volreg.inputs.in_weight_volume = ('functional.nii', 10), you get -weight 'functional.nii[10]'. (I'd add these to the docstring to make sure that it works as I expect.)

@satra
Copy link
Member

satra commented Oct 30, 2017

@williford - could you please merge with current master and update this PR w.r.t @effigies comments?

@effigies
Copy link
Member

Hi @williford. Do you have a few minutes to finish this? It would be nice to have it in for 1.0.

If you don't have the time, I can push my suggested changes to your branch.

Implement suggestions by effigies in
nipy#2155.
@williford
Copy link
Contributor Author

@effigies Sorry for my delay! I just implemented your suggestions, updated the minimal testing, and enabled maintainers to modify my PR. Feel free to make the changes you want.

I did get one error when running tests, but it was unrelated to volreg. The volreg tests passed.

@williford
Copy link
Contributor Author

I failed to fetch the current changes from nipype master branch...

williford added a commit to ck-forks/nipype that referenced this pull request Jan 23, 2018
This is a clean PR, based on the suggestions in
nipy#2155.
@williford
Copy link
Contributor Author

I'm closing this PR, since I created a clean version that is up-to-date with Master here:
#2396

@williford williford closed this Jan 23, 2018
shnizzedy pushed a commit to FCP-INDI/C-PAC that referenced this pull request Nov 8, 2021
This is a clean PR, based on the suggestions in
nipy/nipype#2155.
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.

4 participants