antsRegistrationSyNQuick (continuation of #1392 and #2347) #2453

djarecka · 2018-02-20T19:43:08Z

Had to close #2347 after miscommunication with github...

…dev_syn

…_OF_THREADS env var as num_threads

djarecka · 2018-02-20T21:39:43Z

@satra, I'm not sure if I used environmental variables as num_threads as you were thinking about. I actually don't understand the way it's done in base.py, since LOCAL_DEFAULT_NUMBER_OF_THREADS is a normal global variable defined here.

I decided to check if a user have LOCAL_DEFAULT_NUMBER_OF_THREADS and set to 1 if I can't find it.

satra · 2018-02-20T21:43:32Z

@djarecka - that particular interface overwrites the environment variables, so setting them has no effect. you should just keep your implementation as before with creating a new num_threads field in the inputspec, effectively overwriting the base num_threads.

or, more elegantly, you can keep the base num_threads, but override the parse function and the num_threads sync function.

effigies · 2018-02-20T21:46:53Z

nipype/interfaces/ants/registration.py

+try:
+    LOCAL_DEFAULT_NUMBER_OF_THREADS = int(os.getenv("LOCAL_DEFAULT_NUMBER_OF_THREADS"))
+except TypeError:
+    LOCAL_DEFAULT_NUMBER_OF_THREADS = 1


Let's just import from .base import LOCAL_DEFAULT_NUMBER_OF_THREADS.

Also, I think in general this variable is kind of pointless, unless somebody's using a fork of nipype where it's defined as something other than 1. This isn't something that needs to be fixed in this PR, but just to flag it for others' attention.

effigies · 2018-02-20T21:56:35Z

@satra If you override num_threads (as done here), the Sphinx documentation will include flag: -n %d, which seems more helpful than overriding parsing/syncing functions is elegant.

effigies

Saw a few things (in addition to earlier comment about importing LOCAL_DEFAULT_NUMBER_OF_THREADS) that could use updating.

@satra Please chime in if I'm making suggestions that are out of line with your thoughts on the interface.

Also, please merge master to fix tests.

effigies · 2018-02-21T15:51:49Z

nipype/interfaces/ants/registration.py

+    >>> reg.inputs.num_threads = 2
+    >>> reg.cmdline
+    'antsRegistrationSynQuick.sh -d 3 -f fixed1.nii -m moving1.nii -n 2 -o transform -p d -t s'
+    >>> reg.run()  # doctest: +SKIP


From the help:

NB: Multiple image pairs can be specified for registration during the SyN stage. Specify additional images using the '-m' and '-f' options. Note that image pair correspondence is given by the order specified on the command line. Only the first fixed and moving image pair is used for the linear resgitration stages.

We should add a doctest to make sure that if fixed_image and moving_image are lists, that we see -f fixed1.nii -f fixed2.nii -m moving1.nii -m moving2.nii (or similar).

It may be more trouble than it's worth to verify that the lists are of the same length.

it gives -f fixed1.nii fixed2.nii -m moving1.nii moving2.nii, but i haven't checked if it is properly propagated to the main Registration (couldn't find also examples that confirms that this is a proper command)

Okay, then I think we want to change the argstrs for fixed_image and moving_image to -f %s... and -m %s..., respectively. (The ... will change -f A B to -f A -f B.)

I pushed before reading this comment.

But do you know this is correct syntax of command line? If yes, I can change it, but just wasn't sure what is the correct syntax

Yes. -f A B will fail, while -f A -f B will succeed.

effigies · 2018-02-21T15:54:59Z

nipype/interfaces/ants/registration.py

+            self.inputs.output_prefix + 'InverseWarped.nii.gz')
+        outputs['out_matrix'] = os.path.abspath(self.inputs.output_prefix + '0GenericAffine.mat')
+
+        # todo in the case of linear transformation-only there won't be fields. is there a more elegant way to specify that?


What you have seems like a reasonable approach to me.

effigies · 2018-02-21T15:56:54Z

nipype/interfaces/ants/registration.py

+        outputs = self.output_spec().get()
+        outputs['warped_image'] = os.path.abspath(self.inputs.output_prefix + 'Warped.nii.gz')
+        outputs['inverse_warped_image'] = os.path.abspath(
+            self.inputs.output_prefix + 'InverseWarped.nii.gz')


Just to be a little more concise, maybe we can do the following:

out_base = os.path.abspath(self.inputs.output_prefix) outputs['warped_image'] = out_base + 'Warped.nii.gz' outputs['inverse_warped_image'] = out_base + 'InverseWarped.nii.gz'

and so on...

effigies · 2018-02-21T15:58:18Z

nipype/interfaces/ants/registration.py

+    Examples
+    --------
+
+    >>> import copy, pprint


You're not using these imports

effigies · 2018-02-21T15:58:42Z

nipype/interfaces/ants/registration.py

+    --------
+
+    >>> import copy, pprint
+    >>> from nipype.interfaces.ants import Registration


RegistrationSynQuick

effigies · 2018-02-21T16:04:15Z

nipype/interfaces/ants/registration.py

+    _cmd = 'antsRegistrationSynQuick.sh'
+    input_spec = RegistrationSynQuickInputSpec
+    output_spec = RegistrationSynQuickOutputSpec
+


Just to avoid being affected by upstream changes, we should override _num_threads_update() to do nothing.

def _num_threads_update(self): """antsRegistrationSynQuick.sh ignores environment variables, so override environment update"""

effigies · 2018-02-21T16:10:20Z

nipype/interfaces/ants/registration.py

+
+class RegistrationSynQuick(ANTSCommand):
+    """
+    Reistration using a symmetric image normalization method (SyN).


"Registration" (you're missing a "g").

effigies · 2018-02-23T14:37:28Z

@djarecka Can you "Rerun failed jobs" (in the "Rerun" drop down)? https://circleci.com/workflow-run/6fb99ca4-98b4-4585-92f2-03db3046b4ae

effigies · 2018-02-23T16:18:47Z

Need to make specs.

djarecka · 2018-02-23T17:08:02Z

sorry, forgot to update. should be better now

fliem and others added 7 commits March 3, 2016 20:00

wip: added wrapper for antsRegistrationSyNQuick.sh

83e0aa9

removed wrongly commited file

26aa5d5

Merge branch 'dev_syn' of https://github.com/fliem/nipype into fliem-…

ce5ba21

…dev_syn

applying suggestions from Oscar

7af657a

running check-before-commit

d256a59

small pep8 changes

cc72fbe

Merge remote-tracking branch 'upstream/master' into fliem-dev_syn

66b28df

effigies added this to the 1.0.1 milestone Feb 20, 2018

adding an example to RegistrationSynQuick; using LOCAL_DEFAULT_NUMBER…

fbf24fa

…_OF_THREADS env var as num_threads

effigies reviewed Feb 20, 2018

View reviewed changes

effigies reviewed Feb 21, 2018

View reviewed changes

djarecka added 2 commits February 22, 2018 16:48

Merge branch 'master' into ants_regsynquick

16ed327

typo

b1ea744

after Chris suggestions

3403b0a

effigies approved these changes Feb 23, 2018

View reviewed changes

fixing doctest with multiple images

dbb84a3

update auto test

5cd952f

effigies merged commit 0c6d3f8 into nipy:master Feb 23, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

antsRegistrationSyNQuick (continuation of #1392 and #2347) #2453

antsRegistrationSyNQuick (continuation of #1392 and #2347) #2453

djarecka commented Feb 20, 2018

djarecka commented Feb 20, 2018 •

edited

Loading

satra commented Feb 20, 2018

effigies Feb 20, 2018

effigies commented Feb 20, 2018

effigies left a comment

effigies Feb 21, 2018

djarecka Feb 23, 2018

effigies Feb 23, 2018

djarecka Feb 23, 2018

effigies Feb 23, 2018

effigies Feb 21, 2018

effigies Feb 21, 2018

effigies Feb 21, 2018

effigies Feb 21, 2018

effigies Feb 21, 2018

effigies Feb 21, 2018

djarecka Feb 22, 2018

effigies commented Feb 23, 2018

effigies commented Feb 23, 2018

djarecka commented Feb 23, 2018

antsRegistrationSyNQuick (continuation of #1392 and #2347) #2453

antsRegistrationSyNQuick (continuation of #1392 and #2347) #2453

Conversation

djarecka commented Feb 20, 2018

djarecka commented Feb 20, 2018 • edited Loading

satra commented Feb 20, 2018

Choose a reason for hiding this comment

effigies commented Feb 20, 2018

effigies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

effigies commented Feb 23, 2018

effigies commented Feb 23, 2018

djarecka commented Feb 23, 2018

djarecka commented Feb 20, 2018 •

edited

Loading