Skip to content

Adding niftyreg #1913

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

Merged
merged 20 commits into from
Apr 28, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
a97b291
Initial commit adding the interfaces to the niftyreg executables
mmodat Mar 28, 2017
62134cf
Modifications towards python3
mmodat Mar 29, 2017
44d01b6
Fixing bytes data to string for comparaison for python 3 compatibility
byvernault Mar 29, 2017
37411dd
Editing the workflow to be compatible python 2.7/3.x
byvernault Mar 29, 2017
9e75456
Edit version control
byvernault Mar 29, 2017
b9e37e4
Skipping some test in docstring
byvernault Mar 30, 2017
403947b
Adding # doctest: +SKIP
byvernault Mar 30, 2017
33ebab8
Editing docstring to follow nipype template and add some tests.
byvernault Mar 31, 2017
6800cce
Removing code to raise exceptions for version control and switch to w…
byvernault Apr 3, 2017
cc1c6d0
Editing the tests and docstring
byvernault Apr 3, 2017
18994c6
Adding # doctest: +ALLOW_UNICODE for unicode
byvernault Apr 3, 2017
1ed121d
using files present in testing/data
byvernault Apr 3, 2017
81b738b
moving print to warning and editing docstring
byvernault Apr 4, 2017
4171b19
fixing the last docstring
byvernault Apr 4, 2017
f12762a
Symplifying interfaces using name_source/name_template when possible
byvernault Apr 20, 2017
1527855
Seeting omp to 1 if not set and removing PositiveInt to use traits.Range
byvernault Apr 21, 2017
b84f217
Adding NiftyRegCommandInputSpec and using overload_extension
byvernault Apr 24, 2017
319d6e3
Edit for changes suggested by oesteban
byvernault Apr 26, 2017
61bfeb4
fixing flake8 issue
byvernault Apr 27, 2017
215f5f0
setting self.numthreads to omp_core_val if set
byvernault Apr 28, 2017
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions nipype/interfaces/niftyreg/__init__.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
# -*- coding: utf-8 -*-
# emacs: -*- mode: python; py-indent-offset: 4; indent-tabs-mode: nil -*-
# vi: set ft=python sts=4 ts=4 sw=4 et:

"""
The niftyreg module provides classes for interfacing with the `NiftyReg
<http://sourceforge.net/projects/niftyreg/>`_ command line tools.

Top-level namespace for niftyreg.
"""

from .base import no_niftyreg, get_custom_path
from .reg import RegAladin, RegF3D
from .regutils import (RegResample, RegJacobian, RegAverage, RegTools,
RegTransform, RegMeasure)
128 changes: 128 additions & 0 deletions nipype/interfaces/niftyreg/base.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,128 @@
# -*- coding: utf-8 -*-
# emacs: -*- mode: python; py-indent-offset: 4; indent-tabs-mode: nil -*-
# vi: set ft=python sts=4 ts=4 sw=4 et:

"""
The niftyreg module provides classes for interfacing with `niftyreg
<http://sourceforge.net/projects/niftyreg/>`_ command line tools.

These are the base tools for working with niftyreg.

Registration tools are found in niftyreg/reg.py
Every other tool is found in niftyreg/regutils.py

Examples
--------
See the docstrings of the individual classes for examples.

"""

from __future__ import (print_function, division, unicode_literals,
absolute_import)
from builtins import property, super
from distutils.version import StrictVersion
import os
import shutil
import subprocess
from warnings import warn

from ..base import CommandLine, isdefined, CommandLineInputSpec, traits
from ...utils.filemanip import split_filename


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


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)])


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',
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

argstr='-omp %i')


class NiftyRegCommand(CommandLine):
"""
Base support interface for NiftyReg commands.
"""
_suffix = '_nr'
_min_version = '1.5.30'

def __init__(self, required_version=None, **inputs):
super(NiftyRegCommand, self).__init__(**inputs)
self.required_version = required_version
_version = self.get_version()
if _version:
_version = _version.decode("utf-8")
if StrictVersion(_version) < StrictVersion(self._min_version):
msg = 'A later version of Niftyreg is required (%s < %s)'
warn(msg % (_version, self._min_version))
if required_version is not None:
if StrictVersion(_version) != StrictVersion(required_version):
msg = 'The version of NiftyReg differs from the required'
msg += '(%s != %s)'
warn(msg % (_version, self.required_version))

def check_version(self):
_version = self.get_version()
if not _version:
raise Exception('Niftyreg not found')
# Decoding to string:
_version = _version.decode("utf-8")
if StrictVersion(_version) < StrictVersion(self._min_version):
err = 'A later version of Niftyreg is required (%s < %s)'
raise ValueError(err % (_version, self._min_version))
if self.required_version:
if StrictVersion(_version) != StrictVersion(self.required_version):
err = 'The version of NiftyReg differs from the required'
err += '(%s != %s)'
raise ValueError(err % (_version, self.required_version))

def get_version(self):
if no_niftyreg(cmd=self.cmd):
return None
exec_cmd = ''.join((self.cmd, ' -v'))
return subprocess.check_output(exec_cmd, shell=True).strip()

@property
def version(self):
return self.get_version()

def exists(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi, no problem.

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

Cheers,

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry about that

return self.get_version() is not None

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']):
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Member

Choose a reason for hiding this comment

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

that would be a simple place to do that.

self.inputs.environ['OMP_NUM_THREADS'] = self.num_threads
return super(NiftyRegCommand, self)._run_interface(runtime)

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

def _gen_fname(self, basename, out_dir=None, suffix=None, ext=None):
if basename == '':
msg = 'Unable to generate filename for command %s. ' % self.cmd
msg += 'basename is not set!'
raise ValueError(msg)
_, final_bn, final_ext = split_filename(basename)
if out_dir is None:
out_dir = os.getcwd()
if ext is not None:
final_ext = ext
if suffix is not None:
final_bn = ''.join((final_bn, suffix))
return os.path.abspath(os.path.join(out_dir, final_bn + final_ext))
Loading