-
Notifications
You must be signed in to change notification settings - Fork 524
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
[WIP/ENH] Add antsMotionCorr and related utilities #1812
Conversation
…ill need work, will only handle a single metric type and do basic transformation
…quires tags to metric arguments, and simplified processing of the metric argument formatting.
…puts to antsMotionCorr in list outputs
…added default to dimensionality
… motion parameters
2254440
to
ddaa767
Compare
Hey @rwblair, would it be very hard to re-bump this? Can you merge current master in? (maybe after S3 recovers from the hiccups) |
@oesteban will do |
While ANTsX/ANTs#419 is not solved, can we generate the ITK matrices manually?:
The I will be able to handle you some code to include it. |
Codecov Report
@@ Coverage Diff @@
## master #1812 +/- ##
=========================================
+ Coverage 72.24% 73% +0.75%
=========================================
Files 1088 1066 -22
Lines 55527 53645 -1882
Branches 7994 0 -7994
=========================================
- Hits 40116 39162 -954
- Misses 14145 14483 +338
+ Partials 1266 0 -1266
Continue to review full report at Codecov.
|
nipype/interfaces/ants/preprocess.py
Outdated
|
||
output_average_image = File(hash_files=False, desc="", argstr="%s", | ||
genfile=True, exists=False, usedefault=True) | ||
output_transform_prefix = traits.Str() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please use Str from nipype.interfaces.base
, and add a desc field
nipype/interfaces/ants/preprocess.py
Outdated
|
||
class AntsMotionCorrStatsOutputSpec(TraitedSpec): | ||
''' Output spec for the antsMotionCorrStats command ''' | ||
spatial_map = File(exists=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please, add desc fields to these two outputs
nipype/interfaces/ants/preprocess.py
Outdated
output_average_image = File(hash_files=False, desc="", argstr="%s", | ||
genfile=True, exists=False, usedefault=True) | ||
output_transform_prefix = traits.Str() | ||
output_warped_image = File(hash_files=False, desc="", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please, write desc field
nipype/interfaces/ants/preprocess.py
Outdated
|
||
metric_type = traits.Enum("CC", "MeanSquares", "Demons", "GC", "MI", | ||
"Mattes", argstr="%s") | ||
fixed_image = File(requires=['metric_type'], desc="") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
missing desc
nipype/interfaces/ants/preprocess.py
Outdated
|
||
use_scales_estimator = traits.Bool( | ||
argstr="-e %d", | ||
default=True, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- does
-e
need a value? I think this is just a switch (argstr='-e'
). - Please, set the default value using positional arguments (
traits.Bool(True, argstr=...)
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-e has an argument here:
https://github.com/stnava/ANTs/blob/master/Scripts/antsMotionCorrExample
"template image.") | ||
) | ||
|
||
use_fixed_reference_image = traits.Bool( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- does
-u
need a value? I think this is just a switch (argstr='-u'
). - Please, set the default value using positional arguments (
traits.Bool(True, argstr=...)
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-u has an argument here:
https://github.com/stnava/ANTs/blob/master/Scripts/antsMotionCorrExample
nipype/interfaces/ants/preprocess.py
Outdated
"the dimensionality from the input image." | ||
) | ||
dimensionality = traits.Enum(3, 2, argstr='-d %d', usedefault=True, | ||
position=0, desc=dimension_desc, default=3) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove the final default=3
nipype/interfaces/ants/preprocess.py
Outdated
average_image = File(argstr='-a %s', position=1, exists=False, | ||
desc="Average the input time series image.") | ||
|
||
output_average_image = File(hash_files=False, desc="", argstr="%s", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's no default value, but usedefault=True
. Please fix (either remove usedefault
or set a default value)
nipype/interfaces/ants/preprocess.py
Outdated
c2 = math.sqrt((float(x[2]) * float(x[2])) + (float(x[3]) * float(x[3]))) | ||
t2 = math.atan2(-float(x[4]), c2) | ||
t3 = math.atan2(float(x[3]), float(x[2])) | ||
mat = numpy.zeroes((3, 3)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why don't loading data with numpy.loadtxt, and then use numpy fancy indexing for all of this?
nipype/interfaces/ants/preprocess.py
Outdated
|
||
def _run_interface(self, runtime): | ||
in_fp = open(self.inputs.matrix) | ||
in_data = csv.reader(in_fp) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of this (parsing, reordering, formatting) can be handled by numpy. How about this?
# Ants motion correction output has a single line header that we ignore
# The first column is XXX (volume index?)
in_data = np.loadtxt(self.inputs.matrix, delimiter=',', skiprows=1, usecols=range(1, 14))
translations = in_data[:, 9:]
# There might be a slightly cleaner way to do this
rotations = np.zeros((in_data.shape[0], 3))
for i, row in enumerate(in_data[:, :9]):
rotations[i] = mat2euler(row.reshape(3, 3))
# Don't duplicate this from `_list_outputs`
parameters = self._list_outputs()['parameters']
np.savetxt(parameters, np.hstack((rotations, translations)), fmt='%.8f')
return runtime
nipype/interfaces/ants/preprocess.py
Outdated
exists=True, | ||
desc='Motion crrection matrices to be converted into FSL style motion parameters', | ||
mandatory=True | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for suggesting API changes, but better before merge than after. Feel free to ignore this if I'm being too fiddly.
-
Matrix2FSLParams
doesn't really imply "ANTs MC" to me. I know this is inants/preprocess.py
, but maybe indicate the source a little more clearly?MotionCorr2FSLParams
? -
There's a really handy nipype feature:
class Matrix2FSLParamsInputSpec:
ants_matrix = File(exists=True, mandatory=True,
desc='Motion correction matrices to be converted into FSL-style motion parameters')
fsl_params = File(name_template='%s.par', name_source='ants_matrix',
desc='FSL parameter file')
class Matrix2FSLParamsInputSpec:
fsl_params = File(exists=True, desc='FSL parameter file')
This lets you entirely drop the _list_outputs
function. (You'd need to make the entry in the output spec match the one in the input spec.)
- Also, there's a typo in "correction". And I'd use "FSL-style" instead of "FSL style".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@effigies I think the name_source
only works for command line interfaces. (I remember working on this myself, but I don't recall having it merged).
…converted parameters are close in value
from nipype.utils.tmpdirs import InTemporaryDirectory | ||
from nipype.utils.filemanip import split_filename | ||
|
||
def test_MotionCorr2FSLParams(): | ||
with InTemporaryDirectory(): | ||
cwd = os.getcwd() | ||
fsl_mat_fname = "fsl_style.mat" | ||
fp = open(fsl_mat_fname, 'w+') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
please use numpy.savetxt to workaround encoding issues (py2/3)
"0.000000 0.000000 0.000000 1.000000\n" | ||
) | ||
fp.close() | ||
|
||
in_filename = os.path.join(cwd, 'in_file.csv') | ||
fp = open(in_filename, 'w+') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same as before, please use numpy.savetxt
…t output via numpy works as expected
…into add_antsMotionCorr
@rwblair Do you think you could merge master and update the auto-tests? |
This pull request is "orphaned," which means it has been deemed to be abandoned by its original author. Orphaned pull requests have not been rejected, and we hope that if a user sees one that will meet their needs with a little work, that they will fork it and open a new pull request (or, in the case of the original author, reopen the original PR). We ask that all adopted PRs be updated to merge or rebase the current master. If you would like to adopt a PR and need help getting started, any of a number of contributors will be happy to help. |
Couple of interfaces to use Ants motion correction, and its statistics program. Only included inputs for the affine registration for motion correction.