[FIX] modified afni's cat_matvec to accept empty string opposed to opkey #2943

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

Jump to bottom

Merged

effigies merged 4 commits into nipy:master from ostanley:master

Sep 18, 2019

Merged

[FIX] modified afni's cat_matvec to accept empty string opposed to opkey #2943

effigies merged 4 commits into nipy:master from ostanley:master

Sep 18, 2019

Conversation

@ostanley

Copy link

Contributor

@ostanley ostanley commented Jun 10, 2019 •

edited

Loading

Summary

AFNI's cat_matvec allows for transforms to be concatenated together. The opkey field after each transform allows for things like inversions but is non-mandatory. Currently, if no opkey is specified the - character is added resulting in an error when the interface is run.
Example:
cat_matvec a.1D - b.1D - > c.1D

Reference documentation:
https://afni.nimh.nih.gov/pub/dist/doc/program_help/cat_matvec.html

Fixes

The interface is not able to perform if no opkey is specified aka a list of tuples which are ('file.1D',''). I have fixed this at the format args level.
New behaviour: if an empty opkey is found then the - character is not added.

Acknowledgment

I acknowledge that this contribution will be available under the Apache 2 license.

@ostanley


 modified afni's cat_matvec to accept empty string opposed to opkey

ab4e15a

effigies

effigies reviewed

Sep 16, 2019

View reviewed changes

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.

Thanks for this, I made some suggestions.

nipype/interfaces/afni/utils.py Outdated

xfm_args += ' ' + v[0] + ' -' + v[1] + ' '

else:

xfm_args += ' ' + v[0] + ' '

return spec.argstr % (xfm_args)

Copy link

Member

@effigies effigies Sep 16, 2019

Choose a reason for hiding this comment

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

I think it would be clearer to continue writing this as a comprehension, using the x if y else z idiom, but we can use more verbose variable names to make the logic clearer. I also think it would be useful to include a comment:

# Concatenate a series of filenames, with optional opkeys
return ' '.join('%s -%s' % (mfile, opkey) if opkey else mfile
 for mfile, opkey in value)

I dropped the initial spec.argstr %, just because spec.argstr is "%s", so there's no real point, but you could keep it if you wanted.

Copy link

Contributor Author

@ostanley ostanley Sep 17, 2019

Choose a reason for hiding this comment

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

I agree this is more elegant, I changed it and confirmed it passed my test for formatting.

nipype/interfaces/afni/utils.py

return spec.argstr % (xfm_args)

return super(CatMatvec, self)._format_arg(name, spec, value)

Copy link

Member

@effigies effigies Sep 16, 2019

Choose a reason for hiding this comment

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

Can you re-add this? Python style guidelines suggest 2 lines between classes.

Copy link

Contributor Author

@ostanley ostanley Sep 17, 2019

Choose a reason for hiding this comment

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

Done

@effigies effigies added this to the 1.2.3 milestone

Sep 16, 2019

@ostanley


 made required PR changes to logic and spacing

67b10fb

@codecov

Copy link

codecov bot commented Sep 17, 2019 •

edited

Loading

Codecov Report

Merging #2943 into master will increase coverage by 0.37%.
The diff coverage is 100%.

Impacted file tree graph

@@ Coverage Diff @@
## master #2943 +/- ##
==========================================
+ Coverage 67.14% 67.52% +0.37% 
==========================================
 Files 344 344 
 Lines 44091 44091 
 Branches 5556 5556 
==========================================
+ Hits 29606 29771 +165 
+ Misses 13703 13574 -129 
+ Partials 782 746 -36

Flag	Coverage Δ
#smoketests	`50.3% <ø> (+1.36%)`	⬆️
#unittests	`64.97% <100%> (ø)`	⬆️

Impacted Files	Coverage Δ
nipype/interfaces/afni/utils.py	`81.89% <100%> (ø)`	⬆️
nipype/interfaces/base/core.py	`86.99% <0%> (+0.19%)`	⬆️
nipype/pipeline/engine/workflows.py	`78.95% <0%> (+0.33%)`	⬆️
nipype/pipeline/plugins/legacymultiproc.py	`68% <0%> (+0.5%)`	⬆️
nipype/utils/subprocess.py	`94.02% <0%> (+0.74%)`	⬆️
nipype/utils/filemanip.py	`80.38% <0%> (+0.77%)`	⬆️
nipype/interfaces/fsl/base.py	`87.77% <0%> (+1.11%)`	⬆️
nipype/interfaces/io.py	`54.94% <0%> (+1.33%)`	⬆️
nipype/pipeline/plugins/multiproc.py	`84.56% <0%> (+1.85%)`	⬆️
nipype/interfaces/fsl/preprocess.py	`82.04% <0%> (+2.24%)`	⬆️
... and 5 more

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 154ea61...845463d. Read the comment docs.

@ostanley

Copy link

Contributor Author

ostanley commented Sep 17, 2019

Please let me know if you want any other changes.

effigies

effigies approved these changes

Sep 17, 2019

View reviewed changes

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.

This LGTM. One style nitpick and I think this will be good to go.

nipype/interfaces/afni/utils.py Outdated Show resolved Hide resolved

@effigies

Copy link

Member

effigies commented Sep 17, 2019

The failing tests are due to an issue fixed in #2963. Can you merge master or rebase onto master?

ostanley and others added 2 commits

September 17, 2019 11:38

@ostanley


 Merge branch 'master' of https://github.com/nipy/nipype

88a59ac

@ostanley @effigies


 added style change

845463d

Co-Authored-By: Chris Markiewicz <effigies@gmail.com>

@ostanley ostanley force-pushed the master branch from 2d5586b to 845463d Compare

September 17, 2019 15:43

@effigies

Copy link

Member

effigies commented Sep 18, 2019

Thanks! Merging.

@effigies effigies merged commit 23379eb into nipy:master

Sep 18, 2019

Labels

None yet

2 participants

@ostanley @effigies

[FIX] modified afni's cat_matvec to accept empty string opposed to opkey #2943

[FIX] modified afni's cat_matvec to accept empty string opposed to opkey #2943

Uh oh!

Conversation

@ostanley ostanley commented Jun 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Fixes

Acknowledgment

Uh oh!

@effigies effigies left a comment

Choose a reason for hiding this comment

Uh oh!

@effigies effigies Sep 16, 2019

Choose a reason for hiding this comment

Uh oh!

@ostanley ostanley Sep 17, 2019

Choose a reason for hiding this comment

Uh oh!

@effigies effigies Sep 16, 2019

Choose a reason for hiding this comment

Uh oh!

@ostanley ostanley Sep 17, 2019

Choose a reason for hiding this comment

Uh oh!

codecov bot commented Sep 17, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ostanley commented Sep 17, 2019

Uh oh!

@effigies effigies left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

effigies commented Sep 17, 2019

Uh oh!

effigies commented Sep 18, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

@ostanley ostanley commented Jun 10, 2019 •

edited

Loading

codecov bot commented Sep 17, 2019 •

edited

Loading