Error generated by Automask in nipype.interfaces.afni

[mick-d]

Function Automask raises error due to string '-apply_prefix ' generated in the 3dAutomask command

[mick-d]

Although the fix i proposed seems to work fine, i'm not entire clear what's exactly going on, so any explanation on how this error is generated would be useful before merging this commit.

[mick-d]

I added another PR for using the applyisoxfm option with FSL Flirt (it was not implemented)

[chrisgorgo]

Shouldn't this become part of fsl.ApplyXFM?

[mick-d]

This option actually does not carry out any transformation but simply resamples the image with isotropic voxels (useful for volumes acquired with anisotropic voxel dimensions). This is part of Flirt but this option was not available in NiPype. I was thinking we could also create a separate function Isoresampling using Flirt with apply_isoxfm?

In any case sorry for mixing up these two commits i forgot about my previous PR

[chrisgorgo]

Have a look here:

nipype/nipype/interfaces/fsl/preprocess.py

Line 605 in 9595f27

class ApplyXfm(FLIRT):

On Thu, Jun 27, 2013 at 11:13 AM, Michael notifications@github.com wrote:

In nipype/interfaces/fsl/preprocess.py:

@@ -403,6 +403,8 @@ class FLIRTInputSpec(FSLCommandInputSpec):
in_matrix_file = File(argstr='-init %s', desc='input 4x4 affine matrix')
apply_xfm = traits.Bool(argstr='-applyxfm', requires=['in_matrix_file'],
desc='apply transformation supplied by in_matrix_file')

• apply_isoxfm = traits.Float(argstr='-applyisoxfm %f',

•  desc='as applyxfm but forces isotropic resampling') 

This option actually do not carry out any transformation but simply
resample the image with isotropic voxels (useful when volumes acquired with
anisotropic voxel dimensions). This is part of Flirt but this option was
not available in NiPype. I was thinking we could also create a separate
function Isoresampling using Flirt with apply_isoxfm?

In any case sorry for mixing up these two commits i forgot about my
previous PR

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/586/files#r4917732
.

[mick-d]

Yes i saw that but apply_isoxfm does not carry out any transformation aside resampling (and thus no matrix transformation file required) so why include it in ApplyXFM?

[mick-d]

Or do you mean it should also be added to ApplyXFM to offer the option of isoresampling?

[chrisgorgo]

Two options:

1. Extend ApplyXFM
2. Add as a separate interface (like ApplyIsoXFM)

What do you think @satra ?

[mick-d]

I believe apply_isoxfm is rarely use in conjunction with apply_xfm since the reference volume dictates the final voxel size for standard flirt use (including apply_xfm), that's why i didn't understand what you meant.

On the other hand apply_isoxfm has a very special usage which is usually only to make the image isotropic, using that image as both the source and target volume. This is quite an idosyncratic use so a separate interface would make more sense.

[mick-d]

What about Automask? Is there any chance you could explain me what was wrong exactly?

[satra]

separate interface seems good - how about calling it IsotropicReslicer?

[satra]

is it possible to completely remove _gen_filename in automask?

[mick-d]

IsotropicReslicer or IsoReslicer sounds good!

Although the fix i proposed for automask works, since you are both much more knowledgeable in python (and NiPype obviously) i would be grateful if you could explain me the exact cause of the error. I got mixed up with the use of deprecated fields and didn't understand why the definition of a non-existing argument name such as "deprAcated" didn't create any problem when compiling the code.

[satra]

the reasons are:

• we don't have a test that checks that all metadata in any inputspec are actually valid (i've been meaning to write this, but this keeps getting pushed down the pile)
• the internal check looks only for the presence of the "deprecated" metadata. it doesn't care if a nonsensical metadata is present.

https://github.com/nipy/nipype/blob/master/nipype/interfaces/base.py#L334

[mick-d]

Many thanks Satra. And if this is easy to answer, could you explain in this particular example what line of events led to the parameter "apply_prefix" written twice in the generated Automask command?

[satra]

the lack of a proper "deprecated" perhaps coupled with both inputs being specified. you'll have to post the interface spec and the output for me to decode properly.

[mick-d]

Ok thanks, no worries, i'll look into it in details when i'll have more knowledge of Python, Python libraries and NiPype. Would you like me to do any further change to Automask and FSL Flirt before you accept this PR?

Also sorry to mention it here but do you think you could answer my last post concerning the InputNode? You already wrote quite a few answers but i think my last post explains clearly why there is no obvious interest (if looking only at the tutorials) in using InputNode over directly using DataSource.

Many thanks!

[chrisgorgo]

Could you send the "IsotropicReslicer" as a separate PR and remove 12841c2
commit from this one for clarity? I will look into AFNI troubles soon. Thanks in advance!

[inati]

Chris, before you go too far down into the AFNI troubles, it would be good if we could hash things out in the context of #594. Some of us here at the NIH would like to have input (or at least an understanding) of the architecture. We are in the middle of adding/modifying the wrappers and building some AFNI based pipelines and would be happy to do lifting as long as we're on the same page.

[chrisgorgo]

Yes of course. AFNI and genera refactor of filename generation is a bigger and more complicated issue which will require some more thinking. I don't want to rush it. A new simple interface such as IsotropicReslicer should be easier to review and merge and that's why I want to keep it separate.

[mick-d]

I'll remove the ApplyXFM commit.

It would be good to have the AFNI wrapper bug fixed asap as right now
3dAutomask is unusable despite a very simple temporary fix available since
sime time
On Jul 3, 2013 9:30 AM, "Chris Filo Gorgolewski" notifications@github.com
wrote:

Yes of course. AFNI and genera refactor of filename generation is a bigger
and more complicated issue which will require some more thinking. I don't
want to rush it. A new simple interface such as IsotropicReslicer should be
easier to review and merge and that's why I want to keep it separate.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/586#issuecomment-20415158
.

[inati]

OK but we are making a whole bunch of new interfaces as we speak, so at the very least a bit of documentation that describes the recipe by which you are making an IsotropicReslicer. Maybe we have a different take on the interfaces. For me, I need to have a nipype interface that captures the underlying AFNI tool's interface in a way that is very close to how the AFNI folks think about it. A generic interface that looks the same across tools, i.e. an isotropic reslicer, is an entirely different problem.

[inati]

As far as the 3dAutomask bug, @mick-d has fixed it very much the same way that we have in our branch:
https://github.com/nih-fmrif/nipype/blob/master/nipype/interfaces/afni/preprocess.py#L531
although we were more agressive with the deleting.

[mick-d]

I think there is misunderstanding. There is a very simply fixable bug with
AFNI 3dAutomask wrapper and there is the new feature of isoreslicing with
FSL Flirt which has nothing to do with AFNI.

The confusion most propably arose because I submitted two different PR by
mistake (cf previous comments), sorry about this.
On Jul 3, 2013 9:36 AM, "Souheil Inati" notifications@github.com wrote:

OK but we are making a whole bunch of new interfaces as we speak, so at
the very least a bit of documentation that describes the recipe by which
you are making an IsotropicReslicer. Maybe we have a different take on the
interfaces. For me, I need to have a nipype interface that captures the
underlying AFNI tool's interface in a way that is very close to how the
AFNI folks think about it. A generic interface that looks the same across
tools, i.e. an isotropic reslicer, is an entirely different problem.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/586#issuecomment-20415513
.

[inati]

There is no misunderstanding. The fix to 3dAutomask is simple, but as @mick-d said, understanding why the fix works is not so trivial. We need to have a consensus as to how the interfaces to AFNI functions should look and work. Although that shouldn't stand in the way of merging this bug fix asap.

[mick-d]

That's why I'll remove the FSL
Flirt commit from this PR, as Chris suggested, and it would be good to have
3dAutomask fixed asap for people to be able to use it
On Jul 3, 2013 9:41 AM, "Michael R" mdayan.comp@gmail.com wrote:

I think there is misunderstanding. There is a very simply fixable bug with
AFNI 3dAutomask wrapper and there is the new feature of isoreslicing with
FSL Flirt which has nothing to do with AFNI.

The confusion most propably arose because I submitted two different PR by
mistake (cf previous comments), sorry about this.
On Jul 3, 2013 9:36 AM, "Souheil Inati" notifications@github.com wrote:

OK but we are making a whole bunch of new interfaces as we speak, so at
the very least a bit of documentation that describes the recipe by which
you are making an IsotropicReslicer. Maybe we have a different take on the
interfaces. For me, I need to have a nipype interface that captures the
underlying AFNI tool's interface in a way that is very close to how the
AFNI folks think about it. A generic interface that looks the same across
tools, i.e. an isotropic reslicer, is an entirely different problem.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/586#issuecomment-20415513
.

[mick-d]

Ok I thought the reasons why this fix was working was obvious for everyone
but me, glad to know I'm not the only not 100% clear about the duplicate
parameter origin.

Meanwhile a simple temporary fix would be highly useful as 3dAutomask is
simply unusable right now
On Jul 3, 2013 9:45 AM, "Michael R" mdayan.comp@gmail.com wrote:

That's why I'll remove the FSL
Flirt commit from this PR, as Chris suggested, and it would be good to
have 3dAutomask fixed asap for people to be able to use it
On Jul 3, 2013 9:41 AM, "Michael R" mdayan.comp@gmail.com wrote:

I think there is misunderstanding. There is a very simply fixable bug
with AFNI 3dAutomask wrapper and there is the new feature of isoreslicing
with FSL Flirt which has nothing to do with AFNI.

The confusion most propably arose because I submitted two different PR by
mistake (cf previous comments), sorry about this.
On Jul 3, 2013 9:36 AM, "Souheil Inati" notifications@github.com wrote:

OK but we are making a whole bunch of new interfaces as we speak, so at
the very least a bit of documentation that describes the recipe by which
you are making an IsotropicReslicer. Maybe we have a different take on the
interfaces. For me, I need to have a nipype interface that captures the
underlying AFNI tool's interface in a way that is very close to how the
AFNI folks think about it. A generic interface that looks the same across
tools, i.e. an isotropic reslicer, is an entirely different problem.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/586#issuecomment-20415513
.

[chrisgorgo]

Glad we are on the same page! We'll solve the AFNI issues soon. Now that we have help from someone with more experience with it it should be easy.

[mick-d]

Ok so hopefully this PR can be accepted soon as a temporary fix that would allow people to use 3dAutomask until the set of new AFNI interfaces is ready. Would that work for everyone?
Michael

[chrisgorgo]

@mick-d Can you try https://github.com/chrisfilo/nipype/tree/temp_fix/automask and let me know if it solves the problem for you?

[mick-d]

Hi Chris, sure i can test it but it was not a specific problem of mine, an
error is generated for any call to 3dAutomask.

On Wed, Jul 3, 2013 at 12:54 PM, Chris Filo Gorgolewski <
notifications@github.com> wrote:

@mick-d https://github.com/mick-d Can you try
https://github.com/chrisfilo/nipype/tree/temp_fix/automask and let me
know if it solves the problem for you?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/586#issuecomment-20429431
.

[mick-d]

The tests i did run fine, no duplicate of -apply_prefix parameter generated.

On Wed, Jul 3, 2013 at 1:12 PM, Michael R mdayan.comp@gmail.com wrote:

Hi Chris, sure i can test it but it was not a specific problem of mine, an
error is generated for any call to 3dAutomask.

On Wed, Jul 3, 2013 at 12:54 PM, Chris Filo Gorgolewski <
notifications@github.com> wrote:

@mick-d https://github.com/mick-d Can you try
https://github.com/chrisfilo/nipype/tree/temp_fix/automask and let me
know if it solves the problem for you?

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/586#issuecomment-20429431
.

[chrisgorgo]

Hi again,
Just to give you some sense of closure on the source of this bug and why and how the solution worked. In a nutshell the implementation was broken because it tried maintain backward compatibility while switching to a new way of generating filenames. It was crashing because "apply_mask" had a "source_name" parameter set but no pattern (as in "%s_masked" or something). We could have added the pattern or remove the "source_name" parameter, but since these inputs were advertised to go away in 0.8 I just opted for removal of the deprEcated inputs. Besides there were more things broken about it. All this only shows that we need to rethink and refactor BaseInterface class. I hope this helps.