Adding niftyreg #1913

byvernault · 2017-03-29T13:54:06Z

Dear nipype team,

We are ready to add niftyreg to nipype. I copied all the interfaces we generated for this package as well as the tests. I also copied the workflow (groupwise.py). I did some changes to be compatible python 3 but not sure if everything should be fine for the workflow. If you see anything strange, let me know.

I tested it with python 2 and python 3 for the interfaces. It's working on my computer.

Let me know if you see anything not compatible.

Kind Regards,

Ben

as well as one workflow for groupwise registration

Raising Exception if Niftyreg not found

byvernault · 2017-03-29T15:58:43Z

Hi,

The travis-ci tests failed because we are checking in the base.py for the niftyreg interface that niftyreg is installed followed by checking that the version is bigger than a minimun version.

We are raising an exception if niftyreg not found, the version is smaller than 1.5.10 (min) or version smaller different than a version required.

In the test, the docstring test failed because the system running Travis CI build don't have niftyreg installed so when we create the node in the example, it's failing. Should I just skip testing the docstring?

How should we implement this option to put a version control?

Kind Regards,

oesteban · 2017-03-29T22:04:27Z

@byvernault

For travis CI - we try to keep these tests short, so yes, please decorate the test functions with the @skipif decoration, when niftyreg is not installed.

For circle - we try for these to be very comprehensive, so we would add niftyreg in our Docker image (the file in docker/base.Dockerfile). You'll find there how we get installed a whole bunch of other tools.

Once niftyreg is installed, the tests should not be skipped since it should be available.

Let me know if you need more help with this

byvernault · 2017-03-30T09:06:33Z

Yes, all our tests have the decorator for checking if the executable is installed. The problem came from the docstring in the interfaces because we added some test to check the version in the base interface for NiftyReg.

I added '#doctest: +SKIP' for the docstring.

Cheers,

oesteban · 2017-03-30T17:42:29Z

Sorry, I probably confused you by skip. I think those # doctest: +SKIP are not doing what you would expect.

So, the @skipif directive I recommended is for regression tests (under folders like test/text_**.py) and they will actually run the software.

The doctests in of nipype interfaces should work always. Particularly, you should remove the skip directive on the cmdline so that it is tested.

The only one place where you would like to add # doctest: +SKIP in the docstring is where you have a >>> iface.run() line.

byvernault · 2017-03-31T08:51:42Z

Yeah, we have all the @skipif decorator for the regression tests and it makes sense but I might be confused on the # doctest: +SKIP one. I will try to sort it out.

Thank you @oesteban for the help.

Cheers,

byvernault · 2017-03-31T09:39:17Z

@oesteban , how do you advice to do with the cmdline() in the docstring in the specific case of the output are generated with the fullpath. For example:

Expected:
'fit_asl -source asl.nii.gz -cbf asl_cbf.nii.gz -error asl_error.nii.gz -syn asl_syn.nii.gz'
Got:
u'fit_asl -source asl.nii.gz -cbf /Users/byvernault/home-local/code/git/dev/nipype/nipype/testing/data/asl_cbf.nii.gz -error /Users/byvernault/home-local/code/git/dev/nipype/nipype/testing/data/asl_error.nii.gz -syn /Users/byvernault/home-local/code/git/dev/nipype/nipype/testing/data/asl_syn.nii.gz'

Should I avoid doing the cmdline in the docstring?

Kind Regards,

codecov-io · 2017-03-31T14:44:36Z

Codecov Report

Merging #1913 into master will decrease coverage by 0.41%.
The diff coverage is 51.85%.

@@            Coverage Diff             @@
##           master    #1913      +/-   ##
==========================================
- Coverage   72.55%   72.13%   -0.42%     
==========================================
  Files        1064     1080      +16     
  Lines       54170    55049     +879     
  Branches     7810     7904      +94     
==========================================
+ Hits        39303    39712     +409     
- Misses      13649    14092     +443     
- Partials     1218     1245      +27

Flag	Coverage Δ
#smoketests	`72.13% <51.85%> (-0.42%)`	⬇️
#unittests	`69.72% <51.85%> (-0.31%)`	⬇️

Impacted Files	Coverage Δ
nipype/interfaces/setup.py	`6.89% <0%> (-0.25%)`	⬇️
nipype/interfaces/niftyreg/__init__.py	`100% <100%> (ø)`
nipype/workflows/smri/niftyreg/__init__.py	`100% <100%> (ø)`
...rfaces/niftyreg/tests/test_auto_NiftyRegCommand.py	`100% <100%> (ø)`
nipype/workflows/smri/__init__.py	`100% <100%> (ø)`	⬆️
nipype/interfaces/niftyreg/tests/test_reg.py	`13.88% <13.88%> (ø)`
nipype/interfaces/niftyreg/tests/test_regutils.py	`4.56% <4.56%> (ø)`
nipype/interfaces/niftyreg/base.py	`52% <52%> (ø)`
nipype/workflows/smri/niftyreg/groupwise.py	`8% <8%> (ø)`
nipype/interfaces/niftyreg/regutils.py	`84% <84%> (ø)`
... and 31 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 d387481...215f5f0. Read the comment docs.

oesteban · 2017-03-31T19:45:01Z

Hi @byvernault, look at this example: https://github.com/nipy/nipype/blob/master/nipype/interfaces/fsl/epi.py#L74

So:

interface_object.cmdline # doctest: +ELLIPSIS +ALLOW_UNICODE

Particularly, the +ALLOW_UNICODE directive will make the tests accept that initial u' unicode identifier in python 2.

The +ELLIPSIS allows you to match any characters where you place three dots ..., which is useful for testing on unknown paths.

oesteban · 2017-03-31T19:54:28Z

nipype/interfaces/niftyreg/regutils.py

+        return None
+
+    @property
+    def cmdline(self):


what is the idea behind this? I have the impression that this should be just an option of the input spec.

The reg_average executable as an alternate options to use a command file to describe the command line that the user want to execute. The reason behind this argument is that some cases the command is too long for the terminal to execute it.

Marc designed this argument and we decided to use it by default without the user notifying. Does it make sense?

It's the same thing that using the options but we are not limited by the length of the command line.

I will work on adding the doctests ELLIPSIS.

Thanks,

Cheers,
Ben

byvernault · 2017-04-03T09:19:20Z

Hmm. I have a :

ValueError: line 15 of the doctest for ... has an invalid option: '+ALLOW_UNICODE'

When using the ALLOW_UNICODE and running the nosetests. Am I doing something wrong?

Kind Regards,

Ben

…arning/add function

oesteban · 2017-04-04T02:31:25Z

Hi Ben, sorry I missed this.

It seems you are using it right now. Here, and in the other PRs there are little things to fix.

If you go to https://circleci.com/gh/nipy/nipype/2098?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link and see the detailed description of the crashes, you'll see the following:

most of the command lines are ordered in a different way, so 1) you need to make sure the positional arguments have the position metadata for the File trait of the input spec and 2) once that is fixed, you can copy the order of these arguments to the test because that will be the order in which they are replaced.
some tests need the # doctest: +ELLIPSIS for the paths
some wrong extensions (input images without .gz are tested with .gz)

byvernault · 2017-04-04T15:43:59Z

Hi @oesteban,

Thanks for the help. I think we are getting closer to get this branch ready. I don't see any test failing for the interfaces. Do you have an idea what makes travis-ci to fail?

It says it's the third job for python 2.7.

Kind Regards,
Ben

oesteban · 2017-04-04T15:51:00Z

Hi @byvernault, you hit an open issue we have in travis (#1788). As for CircleCI, I think you caught it in a hiccups, I have restarted that job.

Yes, this is starting to look very good to merge :)

byvernault · 2017-04-04T15:56:19Z

Great. I though I had something that needed to change again :).

Thanks again for all the help.
Let me know if I need to do anything more.

Kind Regards,

byvernault · 2017-04-11T12:48:01Z

Hi @oesteban ,

Do you have an idea of timeline when you are confident about adding our three packages to nipype?
Let me know if I can do anything to help the process of validating niftyreg/niftyseg/niftyfit.

Kind Regards,
Ben

oesteban · 2017-04-19T21:38:59Z

Sorry @byvernault, I'm on vacation right now. I'll try to allocate a few hours to review your PRs next week.

Maybe @satra or @mgxd can take a look on this, so when I look back at it we have some more eyes spotting those things to improve.

Again, my apologies.

byvernault · 2017-04-20T08:35:56Z

Thanks for looking into this. We might need to edit one thing on the niftyfit package. I will try to do so today.

Cheers,

Kind Regards
Ben

satra

i'll post this review, while i look over the workflows. but a lot of the boilerplate operations in the interfaces can be reduced.

satra · 2017-04-20T12:47:06Z

nipype/interfaces/niftyreg/base.py

+    return True
+
+
+class PositiveInt(traits.BaseInt):


while this works, you could have used: a = traits.Range(low=0)

@satra, thx. I just changed all of them to use Range traits instead of rewritting something that does the same.

Cheers,

satra · 2017-04-20T12:49:58Z

nipype/interfaces/niftyreg/base.py

+            final_bn = ''.join((final_bn, suffix))
+        return os.path.abspath(os.path.join(out_dir, final_bn + final_ext))
+
+    def _gen_filename(self, name):


this function, the previous one, and the next one can be removed and the traits extended with name_source and name_template. i'll provide an example when i come across the relevant interface.

Hi @satra , it's used in the interfaces for RegTools for example to avoid repeating some of the code. Should I remove them and put them in each interfaces?

In those interfaces, we name the output depending on the input image and the type specified. Can we use two values with name_source and name_template?

Cheers,

it depends on what you would like to do. there are two things that may help:

keep_extension (traits metadata): allows carrying the extension of name_source through to output.

_overload_extension (class function): here is an example of use in the afni base class. https://github.com/nipy/nipype/blob/master/nipype/interfaces/afni/base.py#L218

@satra, on this part of the interfaces, I left it as it is since for some interfaces, we can not use the name_source since it doesn't allow naming with several variables if I understood the doc well.

For example, Reg_Measure, set the outfile with the basename flo_file where we add the measure_type as suffix and then the extension.

ah i see. while this can indeed be done in overload_extension, we will likely have a cleaner mechanism in the next release to support multiple variables.

Great to know.

Do you want us to use the overload_extension in all those cases or leave it like that?

i would prefer overload extension over gen_filename at this point. when we implement the multiple variables option, it will simply allow us to improve the namesource and template.

Ok, working on it.

satra · 2017-04-20T12:55:39Z

nipype/interfaces/niftyreg/reg.py

+    verbosity_off_flag = traits.Bool(argstr='-voff',
+                                     desc='Turn off verbose output')
+    # Set the number of omp thread to use
+    omp_core_val = traits.Int(desc='Number of openmp thread to use',


how does this work if OMP_NUM_THREADS is set? will this override it?

Yes. If the omp_core_val is set, it is used, otherwise the environment variable. If none are set, the maximum number of thread is used, as defined by omp_get_num_procs() in C.

thanks @mmodat - in nipype there are a few interfaces that use OMP (AFNI, ITK for example). in addition to setting a parameter such as this, you may want to override OMP_NUM_THREADS as well, such as here: https://github.com/nipy/nipype/blob/master/nipype/interfaces/afni/base.py#L190

hopefully over the next iteration, we will achieve a coherent numthreads setting independent of whether this means OMP, MKL, python threads, cores on a cluster, etc.,.

Thanks @satra for the suggestion. However, if you agree, we rather leave it has it is here at it gives us more flexibility within our workflows. We included code similar to the one in AFNI but at the level of the workflow rather than at the level of the executables.

@mmodat - you can leave that input as is, but to interact with other pieces of nipype, you should set the interfaces self.num_threads to the same value as the input always. also the default value of that input should be 1. i.e. every interface should by default run with one thread and it's up to the user/workflow designer to alter it.

@satra Sounds good, we will alter it then.

@satra,

what do you think about the changes done?
def _run_interface(self, runtime):
# Update num threads estimate from OMP_NUM_THREADS env var
# Default to 1 if not set
if not isdefined(self.inputs.environ['OMP_NUM_THREADS']):
self.inputs.environ['OMP_NUM_THREADS'] = self.num_threads
return super(NiftyRegCommand, self)._run_interface(runtime)

We set the OMP_NUM_THREADS to the number of threads if not set.

@byvernault - they look good. (i suspect that you haven't hit the other files yet.

and this _run_interface looks good. you may want to consider moving omp_core_val to a base class inputspec and creating this _run_interface function in the base class. seems like all your interfaces support it.

i would also add the following at the beginning of run interface.

if isdefined(self.inputs.omp_core_val): self.num_threads = self.inputs.omp_core_val

I was thinking about adding this as well but actually in the code of all the executables, it reads this value if set. It means that even if OMP_NUM_THREADS is set, it won't be used by the executables since the options is set by the user.

I was considering about doing the inputspec for all the interfaces with the omp_core_val but I haven't done it yet.

satra · 2017-04-20T13:02:14Z

nipype/interfaces/niftyreg/reg.py

+                              argstr='-omp %i')
+
+    # Affine output transformation matrix file
+    aff_file = File(genfile=True,


aff_file = File(name_source=['flo_file'], name_template='%s_aff.txt', desc='The output affine matrix file', argstr='-aff %s')

similarly for res_file below.

satra · 2017-04-20T13:02:37Z

nipype/interfaces/niftyreg/reg.py

+    input_spec = RegAladinInputSpec
+    output_spec = RegAladinOutputSpec
+
+    def _gen_filename(self, name):


as a result of name source and template this function can be removed.

satra · 2017-04-20T13:05:45Z

nipype/interfaces/niftyreg/reg.py

+        return None
+
+    def _list_outputs(self):
+        outputs = self.output_spec().get()


this function can be reduced to:

def _list_outputs(self): outputs = super(RegAladin, self)._list_outputs() # Make a list of the linear transformation file and the input image aff = os.path.abspath(outputs['aff_file']) flo = os.path.abspath(self.inputs.flo_file) outputs['avg_output'] = '%s %s' % (aff, flo) return outputs

it seems that the avg_output here is combining things to make it easy for some other interface. this breaks the dataflow paradigm as the next interface will not be able to use such a string to determine the hash of the input properly. it would be better to do such a merge separately in a workflow rather than as an output.

byvernault · 2017-04-20T15:03:47Z

Thx @satra, I will work on doing the changes you requested.
I will extend it to the other interfaces for niftyseg/niftyfit.

Kind Regards,

satra · 2017-04-24T14:18:28Z

nipype/interfaces/niftyreg/regutils.py

+    output_spec = RegJacobianOutputSpec
+
+    def _gen_filename(self, name):
+        if name == 'out_file':


fix here as well

How should I proceed to name the file with name_source/name_template in the case of:
the file will be named: '{0}_{1}.ext'.format(self.inputs.trans_file, self.inputs.type)?

byvernault · 2017-04-24T16:10:05Z

I think I did all the changes. Let me know if you see something else that you want me to edit. Thank you for your help.

Do you have a rough idea when you will be able to merge the requests to nipype?

Kind Regards.
Ben

oesteban

I have suggested some changes, but this contribution is great already. Thanks a lot!

oesteban · 2017-04-25T20:57:13Z

nipype/interfaces/niftyreg/base.py

+
+def get_custom_path(command):
+    try:
+        specific_dir = os.environ['NIFTYREGDIR']


what about:

def get_custom_path(command): return os.path.join(os.getenv('NIFTYREGDIR', ''), command)

yes, I should have changed this before to what you have. Thx. I will edit it.

oesteban · 2017-04-25T21:04:13Z

nipype/interfaces/niftyreg/base.py

+
+
+def no_niftyreg(cmd='reg_f3d'):
+    if True in [os.path.isfile(os.path.join(path, cmd)) and


True in [...] should be replaced by any:

import shutil def no_niftyreg(cmd='reg_f3d'): try: return shutil.which(cmd) is None except AttributeError: # Python < 3.3 return not any([os.path.isfile(os.path.join(path, cmd)) and os.access(os.path.join(path, cmd), os.X_OK) for path in os.environ["PATH"].split(os.pathsep)]

@satra, I think we should provide an all-python compatible which function in filemanip. WDYT?

Sure, it makes sense.

oesteban · 2017-04-25T21:05:29Z

nipype/interfaces/niftyreg/base.py

+class NiftyRegCommandInputSpec(CommandLineInputSpec):
+    """Input Spec for niftyreg interfaces."""
+    # Set the number of omp thread to use
+    omp_core_val = traits.Int(desc='Number of openmp thread to use',


should we have a default value here? What is the default in niftyreg?

We didn't set a default value because if this is not set by the user, we are using the OMP_NUM_THREADS value.

oesteban · 2017-04-25T21:07:00Z

nipype/interfaces/niftyreg/base.py

+    def version(self):
+        return self.get_version()
+
+    def exists(self):


def exists(self): return not self.get_version() is None

Sorry to butt in, but not x is None should be x is not None. Just a style issue, but a lot of style checkers will yell at you.

Speaking of which, have you run all of this through a style checker, such as flake8? Nipype has somewhat intermittent style compliance, but for new contributions it's good to start off on the right foot.

Hi, no problem.

I am using flake8 but it does not complain with this one. I will edit it.

Cheers,

Sorry about that

oesteban · 2017-04-25T21:08:46Z

nipype/interfaces/niftyreg/base.py

+    def _run_interface(self, runtime):
+        # Update num threads estimate from OMP_NUM_THREADS env var
+        # Default to 1 if not set
+        if not isdefined(self.inputs.environ['OMP_NUM_THREADS']):


I think you can remove this if you made:

omp_core_val = traits.Int(1, usedefault=True, desc='Number of openmp thread to use', argstr='-omp %i')

Up there in the input spec definition.

For omp_core_val, if it's fine, we would like to leave it like that without having a default value and set it to one by the env variable.

On our hand, we have other scripts where when we run the workflow, we can specify the number of cores for qsub and we set the OMP_NUM_THREADS. If we set by default the value of omp_core_val, then it won't read the value set by OMP_NUM_THREADS.

If really needed, we can set it to the value of OMP_NUM_THREADS if set or 1.

What do you think?

the resource multiproc plugin uses the numthreads variable to determine whether it will schedule a process on a multicore machine. by setting this variable when omp_core_val or OMP_NUM_THREADS is set, you make the process more efficient by aligning requests with allocation.

@byvernault - if you can take care of this, and we can make sure all the tests pass this should be ready for merge.

Sounds good. How do you want me to proceed?
Do you want me to set this in the _format_arg ?

def _format_arg(self, name, spec, value):
if name == 'omp_core_val':
self.numthreads = value
return super(NiftyRegCommand, self)._format_arg(name, spec, value)

that would be a simple place to do that.

oesteban · 2017-04-25T21:13:35Z

nipype/interfaces/niftyreg/reg.py

+-res im2_res.nii.gz -rmask mask.nii'
+
+    """
+    _cmd = get_custom_path('reg_f3d')


@satra: this is interesting, maybe we should provide a more reliable way of finding binaries. There is the which util we could add to filemanip, but also, this is very interesting - to allow users to build absolute paths from environment variables. WDYT?

@oesteban - i thought that this was an interesting approach and one that could be extended to support containers as well. let's merge this PR in before this current release (tomorrow) and then think about the few additional pieces (namesource and this custom path updates) for the next release.

@satra I just recalled we have nipype.interfaces.base._exists_in_path. We could place this one in filemanip (where it should probably live), and make use of shutil.which for python > 3.3.

WDYT?

satra · 2017-04-28T22:18:40Z

all other tests pass and the mapnode crash is preventing on py27 test.

mmodat and others added 5 commits March 28, 2017 10:44

Initial commit adding the interfaces to the niftyreg executables

a97b291

as well as one workflow for groupwise registration

Modifications towards python3

62134cf

Fixing bytes data to string for comparaison for python 3 compatibility

44d01b6

Editing the workflow to be compatible python 2.7/3.x

37411dd

Edit version control

9e75456

Raising Exception if Niftyreg not found

byvernault added 2 commits March 30, 2017 09:37

Skipping some test in docstring

b9e37e4

Adding # doctest: +SKIP

403947b

Editing docstring to follow nipype template and add some tests.

33ebab8

oesteban reviewed Mar 31, 2017

View reviewed changes

byvernault added 4 commits April 3, 2017 11:23

Removing code to raise exceptions for version control and switch to w…

6800cce

…arning/add function

Editing the tests and docstring

cc1c6d0

Adding # doctest: +ALLOW_UNICODE for unicode

18994c6

using files present in testing/data

1ed121d

byvernault added 2 commits April 4, 2017 10:08

moving print to warning and editing docstring

81b738b

fixing the last docstring

4171b19

satra requested changes Apr 20, 2017

View reviewed changes

byvernault added 2 commits April 20, 2017 18:38

Symplifying interfaces using name_source/name_template when possible

f12762a

Seeting omp to 1 if not set and removing PositiveInt to use traits.Range

1527855

satra reviewed Apr 24, 2017

View reviewed changes

Adding NiftyRegCommandInputSpec and using overload_extension

b84f217

oesteban approved these changes Apr 25, 2017

View reviewed changes

Edit for changes suggested by oesteban

319d6e3

mgxd added the in-progress label Apr 26, 2017

byvernault added 2 commits April 27, 2017 13:07

fixing flake8 issue

61bfeb4

setting self.numthreads to omp_core_val if set

215f5f0

oesteban mentioned this pull request Apr 28, 2017

Adding niftyseg #1911

Merged

satra merged commit ec6643a into nipy:master Apr 28, 2017



		def no_niftyreg(cmd='reg_f3d'):
		if True in [os.path.isfile(os.path.join(path, cmd)) and

Adding niftyreg #1913

Adding niftyreg #1913

Uh oh!

Conversation

byvernault commented Mar 29, 2017

Uh oh!

byvernault commented Mar 29, 2017

Uh oh!

oesteban commented Mar 29, 2017

Uh oh!

byvernault commented Mar 30, 2017

Uh oh!

oesteban commented Mar 30, 2017

Uh oh!

byvernault commented Mar 31, 2017

Uh oh!

byvernault commented Mar 31, 2017

Uh oh!

codecov-io commented Mar 31, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

oesteban commented Mar 31, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

byvernault commented Apr 3, 2017

Uh oh!

oesteban commented Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

byvernault commented Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oesteban commented Apr 4, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

byvernault commented Apr 4, 2017

Uh oh!

byvernault commented Apr 11, 2017

Uh oh!

oesteban commented Apr 19, 2017

Uh oh!

byvernault commented Apr 20, 2017

Uh oh!

satra left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

byvernault Apr 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov-io commented Mar 31, 2017 •

edited

Loading

oesteban commented Apr 4, 2017 •

edited

Loading

byvernault commented Apr 4, 2017 •

edited

Loading

oesteban commented Apr 4, 2017 •

edited

Loading

byvernault Apr 24, 2017 •

edited

Loading

byvernault Apr 24, 2017 •

edited

Loading

byvernault Apr 26, 2017 •

edited

Loading