Skip to content

changes in Interfaces base (closes #2320) #2387

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 11 commits into from
Jan 22, 2018
8 changes: 6 additions & 2 deletions nipype/interfaces/base/core.py
Original file line number Diff line number Diff line change
Expand Up @@ -411,8 +411,12 @@ def _check_version_requirements(self, trait_object, raise_exception=True):
raise Exception(
'Trait %s (%s) (version %s < required %s)' %
(name, self.__class__.__name__, version, min_ver))
check = dict(max_ver=lambda t: t is not None)
names = trait_object.trait_names(**check)

# check maximum version
check = dict(max_ver=lambda t: t is not None)
names = trait_object.trait_names(**check)
if names and self.version:
version = LooseVersion(str(self.version))
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I see a semantic difference in 955b3ad. I see:

if names and self.version:
    ...  # Check min_ver
    check = dict(...)
    names = trait_object.trait_names(**check)
    ...  # Check max_ver

Which becomes:

if names and self.version:
    ...  # Check min_ver

check = dict(...)
names = trait_object.trait_names(**check)

if names and self.version:
    ...  # Check max_ver

Am I missing something?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

answered lower in the discussion

for name in names:
max_ver = LooseVersion(
str(trait_object.traits()[name].max_ver))
Expand Down
37 changes: 0 additions & 37 deletions nipype/interfaces/base/specs.py
Original file line number Diff line number Diff line change
Expand Up @@ -119,20 +119,6 @@ def _xor_warn(self, obj, name, old, new):
'which is already set') % (name, trait_name)
raise IOError(msg)

def _requires_warn(self, obj, name, old, new):
Copy link
Member

Choose a reason for hiding this comment

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

This used to be called from _generate_handlers.

for elem in requires:
self.on_trait_change(self._requires_warn, elem)

I was unable to quickly find the PR it was removed in, so it's hard to tell whether its removal was intentional or a mistake. As an unused private methods, I'm okay with getting rid of it, but @satra, if this should still be in _generate_handlers, let us know.

"""Part of the xor behavior
"""
if isdefined(new):
trait_spec = self.traits()[name]
msg = None
for trait_name in trait_spec.requires:
if not isdefined(getattr(self, trait_name)):
if not msg:
msg = 'Input %s requires inputs: %s' \
% (name, ', '.join(trait_spec.requires))
if msg: # only one requires warning at a time.
warn(msg)

def _deprecated_warn(self, obj, name, old, new):
"""Checks if a user assigns a value to a deprecated trait
"""
Expand Down Expand Up @@ -165,29 +151,6 @@ def _deprecated_warn(self, obj, name, old, new):
'%s' % trait_spec.new_name: new
})

def _hash_infile(self, adict, key):
Copy link
Member

Choose a reason for hiding this comment

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

Unused private method. Good to remove, as far as I'm concerned. Again, @satra, feel free to override if this looks like a mistake.

""" Inject file hashes into adict[key]"""
stuff = adict[key]
if not is_container(stuff):
stuff = [stuff]
file_list = []
for afile in stuff:
if is_container(afile):
hashlist = self._hash_infile({'infiles': afile}, 'infiles')
hash = [val[1] for val in hashlist]
else:
if config.get('execution',
'hash_method').lower() == 'timestamp':
hash = hash_timestamp(afile)
elif config.get('execution',
'hash_method').lower() == 'content':
hash = hash_infile(afile)
else:
raise Exception("Unknown hash method: %s" % config.get(
'execution', 'hash_method'))
file_list.append((afile, hash))
return file_list

def get(self, **kwargs):
""" Returns traited class as a dict

Expand Down
6 changes: 2 additions & 4 deletions nipype/interfaces/base/support.py
Original file line number Diff line number Diff line change
Expand Up @@ -168,10 +168,8 @@ def _get_bunch_hash(self):
sorted_dict = to_str(sorted(dict_nofilename.items()))
return dict_withhash, md5(sorted_dict.encode()).hexdigest()

def __pretty__(self, p, cycle):
"""Support for the pretty module

pretty is included in ipython.externals for ipython > 0.10"""
def _repr_pretty_(self, p, cycle):
Copy link
Member

Choose a reason for hiding this comment

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

I'm okay with this, but we should verify that we can pretty print Bunches in IPython with this change. I don't know if the API changed beyond what method it looks for in your objects.

if it doesn't work quickly, I'd say remove the function altogether and create an issue to fix and re-add, rather than waste time on this today.

Copy link
Collaborator Author

@djarecka djarecka Jan 22, 2018

Choose a reason for hiding this comment

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

I've just checked that pretty from IPython.lib.pretty uses _repr_pretty_ method (and completely ignored when it was named __pretty__).
In my opinion it looks as "pretty" as simply printing Bunch, so I can either remove it or keep my name.

Let me know if this was design to work with different library.

Copy link
Member

Choose a reason for hiding this comment

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

It looks like what it does is prevents an infinite loop during formating. Could you test with a bunch that contains itself?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes, you're right, for a bunch that contains itself it makes a difference! But only with the new name _repr_pretty_.
And you don't actually have to import pretty in ipython.

"""Support for the pretty module from ipython.externals"""
if cycle:
p.text('Bunch(...)')
else:
Expand Down
52 changes: 27 additions & 25 deletions nipype/interfaces/base/tests/test_core.py
Original file line number Diff line number Diff line change
Expand Up @@ -177,74 +177,76 @@ def __init__(self, **inputs):
assert 'ec5755e07287e04a4b409e03b77a517c' == hashvalue


def test_input_version():
class InputSpec(nib.TraitedSpec):
foo = nib.traits.Int(desc='a random int', min_ver='0.9')
class MinVerInputSpec(nib.TraitedSpec):
foo = nib.traits.Int(desc='a random int', min_ver='0.9')

class MaxVerInputSpec(nib.TraitedSpec):
foo = nib.traits.Int(desc='a random int', max_ver='0.7')


def test_input_version_1():
class DerivedInterface1(nib.BaseInterface):
input_spec = InputSpec
input_spec = MinVerInputSpec

obj = DerivedInterface1()
obj._check_version_requirements(obj.inputs)

config.set('execution', 'stop_on_unknown_version', True)

with pytest.raises(Exception):
with pytest.raises(ValueError) as excinfo:
obj._check_version_requirements(obj.inputs)
assert "no version information" in str(excinfo.value)

config.set_default_config()

class InputSpec(nib.TraitedSpec):
foo = nib.traits.Int(desc='a random int', min_ver='0.9')

def test_input_version_2():
class DerivedInterface1(nib.BaseInterface):
input_spec = InputSpec
input_spec = MinVerInputSpec
_version = '0.8'

obj = DerivedInterface1()
obj.inputs.foo = 1
with pytest.raises(Exception):
obj._check_version_requirements()
with pytest.raises(Exception) as excinfo:
obj._check_version_requirements(obj.inputs)
assert "version 0.8 < required 0.9" in str(excinfo.value)

class InputSpec(nib.TraitedSpec):
foo = nib.traits.Int(desc='a random int', min_ver='0.9')

def test_input_version_3():
class DerivedInterface1(nib.BaseInterface):
input_spec = InputSpec
input_spec = MinVerInputSpec
_version = '0.10'

obj = DerivedInterface1()
obj._check_version_requirements(obj.inputs)

class InputSpec(nib.TraitedSpec):
foo = nib.traits.Int(desc='a random int', min_ver='0.9')

def test_input_version_4():
class DerivedInterface1(nib.BaseInterface):
input_spec = InputSpec
input_spec = MinVerInputSpec
_version = '0.9'

obj = DerivedInterface1()
obj.inputs.foo = 1
obj._check_version_requirements(obj.inputs)

class InputSpec(nib.TraitedSpec):
foo = nib.traits.Int(desc='a random int', max_ver='0.7')

def test_input_version_5():
class DerivedInterface2(nib.BaseInterface):
input_spec = InputSpec
input_spec = MaxVerInputSpec
_version = '0.8'

obj = DerivedInterface2()
obj.inputs.foo = 1
with pytest.raises(Exception):
obj._check_version_requirements()
with pytest.raises(Exception) as excinfo:
obj._check_version_requirements(obj.inputs)
assert "version 0.8 > required 0.7" in str(excinfo.value)

class InputSpec(nib.TraitedSpec):
foo = nib.traits.Int(desc='a random int', max_ver='0.9')

def test_input_version_6():
class DerivedInterface1(nib.BaseInterface):
input_spec = InputSpec
_version = '0.9'
input_spec = MaxVerInputSpec
_version = '0.7'

obj = DerivedInterface1()
obj.inputs.foo = 1
Expand Down
103 changes: 9 additions & 94 deletions nipype/interfaces/base/traits_extension.py
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ class Str(Unicode):
traits.DictStrStr = DictStrStr


class BaseFile(BaseUnicode):
class File(BaseUnicode):
Copy link
Member

Choose a reason for hiding this comment

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

I'm unclear on the original justification for this separation, but dropping BaseFile seems fine to me.

""" Defines a trait whose value must be the name of a file.
"""

Expand Down Expand Up @@ -96,14 +96,11 @@ def __init__(self,
if exists:
self.info_text = 'an existing file name'

super(BaseFile, self).__init__(value, **metadata)
super(File, self).__init__(value, **metadata)

def validate(self, object, name, value):
""" Validates that a specified value is valid for this trait.

Note: The 'fast validator' version performs this check in C.
"""
validated_value = super(BaseFile, self).validate(object, name, value)
""" Validates that a specified value is valid for this trait."""
validated_value = super(File, self).validate(object, name, value)
if not self.exists:
return validated_value
elif os.path.isfile(value):
Expand All @@ -117,53 +114,12 @@ def validate(self, object, name, value):
self.error(object, name, value)


class File(BaseFile):
"""
Defines a trait whose value must be the name of a file.
Disables the default C-level fast validator.
"""

def __init__(self,
value='',
filter=None,
auto_set=False,
entries=0,
exists=False,
**metadata):
""" Creates a File trait.

Parameters
----------
value : string
The default value for the trait
filter : string
A wildcard string to filter filenames in the file dialog box used by
the attribute trait editor.
auto_set : boolean
Indicates whether the file editor updates the trait value after
every key stroke.
exists : boolean
Indicates whether the trait value must be an existing file or
not.

Default Value
-------------
*value* or ''
"""
# if not exists:
# # Define the C-level fast validator to use:
# fast_validate = (11, str)

super(File, self).__init__(value, filter, auto_set, entries, exists,
**metadata)


# -------------------------------------------------------------------------------
# 'BaseDirectory' and 'Directory' traits:
# 'Directory' trait
# -------------------------------------------------------------------------------


class BaseDirectory(BaseUnicode):
class Directory(BaseUnicode):
"""
Defines a trait whose value must be the name of a directory.
"""
Expand All @@ -177,7 +133,7 @@ def __init__(self,
entries=0,
exists=False,
**metadata):
""" Creates a BaseDirectory trait.
""" Creates a Directory trait.

Parameters
----------
Expand All @@ -201,13 +157,10 @@ def __init__(self,
if exists:
self.info_text = 'an existing directory name'

super(BaseDirectory, self).__init__(value, **metadata)
super(Directory, self).__init__(value, **metadata)

def validate(self, object, name, value):
""" Validates that a specified value is valid for this trait.

Note: The 'fast validator' version performs this check in C.
"""
""" Validates that a specified value is valid for this trait."""
if isinstance(value, (str, bytes)):
if not self.exists:
return value
Expand All @@ -222,44 +175,6 @@ def validate(self, object, name, value):
self.error(object, name, value)


class Directory(BaseDirectory):
"""
Defines a trait whose value must be the name of a directory.
Disables the default C-level fast validator.
"""

def __init__(self,
value='',
auto_set=False,
entries=0,
exists=False,
**metadata):
""" Creates a Directory trait.

Parameters
----------
value : string
The default value for the trait
auto_set : boolean
Indicates whether the directory editor updates the trait value
after every key stroke.
exists : boolean
Indicates whether the trait value must be an existing directory or
not.

Default Value
-------------
*value* or ''
"""
# Define the C-level fast validator to use if the directory existence
# test is not required:
# if not exists:
# self.fast_validate = (11, str)

super(Directory, self).__init__(value, auto_set, entries, exists,
**metadata)


# lists of tuples
# each element consists of :
# - uncompressed (tuple[0]) extension
Expand Down
3 changes: 0 additions & 3 deletions nipype/pipeline/engine/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -1158,9 +1158,6 @@ def _standardize_iterables(node):
iterables = node.iterables
# The candidate iterable fields
fields = set(node.inputs.copyable_trait_names())
# Flag indicating whether the iterables are in the alternate
# synchronize form and are not converted to a standard format.
# synchronize = False # OE: commented out since it is not used
# A synchronize iterables node without an itersource can be in
# [fields, value tuples] format rather than
# [(field, value list), (field, value list), ...]
Expand Down
1 change: 0 additions & 1 deletion nipype/utils/misc.py
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,6 @@ def dict_diff(dold, dnew, indent=0):
# Values in common keys would differ quite often,
# so we need to join the messages together
for k in new_keys.intersection(old_keys):
same = False
try:
new, old = dnew[k], dold[k]
same = new == old
Expand Down