Skip to content

FIX: AddCSVRow failed when using infields #1028

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.

Sign up for GitHub

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 9 commits into from
Feb 14, 2015
Merged

FIX: AddCSVRow failed when using infields #1028

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e98181b
fix AddCSVRow when using infields
oesteban Jan 22, 2015
cb9e220
update CHANGES
oesteban Jan 22, 2015
4c7e452
Merge branch 'master' into fix/AddCSVRowUndefined
oesteban Feb 9, 2015
58778a7
fix docstring
oesteban Feb 9, 2015
9f881bb
Merge branch 'master' into fix/AddCSVRowUndefined
oesteban Feb 10, 2015
84e5410
Merge branch 'master' into fix/AddCSVRowUndefined
oesteban Feb 11, 2015
6498abc
Merge branch 'master' into fix/AddCSVRowUndefined
oesteban Feb 13, 2015
769dece
use copyable_trait_names
oesteban Feb 13, 2015
e53fa06
add the possibly new code using portalocker in comment
oesteban Feb 13, 2015
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
1 change: 1 addition & 0 deletions CHANGES
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
Next release
============

* FIX: AddCSVRow problems when using infields (https://github.com/nipy/nipype/pull/1028)
* ENH: New io interfaces for JSON files reading/writing (https://github.com/nipy/nipype/pull/1020)
* ENH: Enhanced openfmri script to support freesurfer linkage (https://github.com/nipy/nipype/pull/1037)
* BUG: matplotlib is supposed to be optional (https://github.com/nipy/nipype/pull/1003)
Expand Down
29 changes: 18 additions & 11 deletions nipype/algorithms/misc.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@
from ..interfaces.base import (BaseInterface, traits, TraitedSpec, File,
InputMultiPath, OutputMultiPath,
BaseInterfaceInputSpec, isdefined,
DynamicTraitedSpec)
DynamicTraitedSpec, Undefined)
from nipype.utils.filemanip import fname_presuffix, split_filename
iflogger = logging.getLogger('interface')

Expand Down Expand Up @@ -785,7 +785,8 @@ def _list_outputs(self):


class AddCSVRowInputSpec(DynamicTraitedSpec, BaseInterfaceInputSpec):
in_file = traits.File(mandatory=True, desc='Input comma-separated value (CSV) files')
in_file = traits.File(mandatory=True,
desc='Input comma-separated value (CSV) files')
_outputs = traits.Dict(traits.Any, value={}, usedefault=True)

def __setattr__(self, key, value):
Expand All @@ -804,13 +805,15 @@ class AddCSVRowOutputSpec(TraitedSpec):


class AddCSVRow(BaseInterface):

"""Simple interface to add an extra row to a csv file

.. note:: Requires `pandas <http://pandas.pydata.org/>`_

.. warning:: Multi-platform thread-safe execution is possible with
`lockfile <https://pythonhosted.org/lockfile/lockfile.html>`_. Please recall that (1)
this module is alpha software; and (2) it should be installed for thread-safe writing.
`lockfile <https://pythonhosted.org/lockfile/lockfile.html>`_. Please
recall that (1) this module is alpha software; and (2) it should be
installed for thread-safe writing.
If lockfile is not installed, then the interface is not thread-safe.


Expand All @@ -822,7 +825,7 @@ class AddCSVRow(BaseInterface):
>>> addrow.inputs.in_file = 'scores.csv'
>>> addrow.inputs.si = 0.74
>>> addrow.inputs.di = 0.93
>>> addrow.subject_id = 'S400'
>>> addrow.inputs.subject_id = 'S400'
>>> addrow.inputs.list_of_values = [ 0.4, 0.7, 0.3 ]
>>> addrow.run() # doctest: +SKIP
"""
Expand Down Expand Up @@ -850,22 +853,26 @@ def _run_interface(self, runtime):
try:
import pandas as pd
except ImportError:
raise ImportError('This interface requires pandas (http://pandas.pydata.org/) to run.')
raise ImportError(('This interface requires pandas '
'(http://pandas.pydata.org/) to run.'))

try:
import lockfile as pl
Copy link
Member

@satra satra Feb 11, 2015

Choose a reason for hiding this comment

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

i know this wasn't the change here, but in nipype.externals there is a lock file code that might work across multiprocessor and possibly in nfs settings. in nfs, the default cache time could be 30s or higher, so the lock may not propagate across nodes without timeout settings in place.

Copy link
Contributor Author

@oesteban oesteban Feb 13, 2015

Choose a reason for hiding this comment

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

@satra, I've been looking at portalocker (http://portalocker.readthedocs.org/en/latest/usage.html#examples) included in nipype.externals, and before integrating it in this interface I have a minor concern:

  • I think that the script included in nipype.externals is a bit outdated. The new version includes the class Lock that can be used with contexts. Therefore, I would update the external code to this new version.

I would proceed with this PR as it fixes a bug on this interface. Then update portalocker and finally integrate it in all io classes.

self._have_lock = True
except ImportError:
import warnings
warnings.warn(('Python module lockfile was not found: AddCSVRow will not be thread-safe '
'in multi-processor execution'))
from warnings import warn
warn(('Python module lockfile was not found: AddCSVRow will not be'
' thread-safe in multi-processor execution'))

input_dict = {}
for key, val in self.inputs._outputs.items():
# expand lists to several columns
if key == 'trait_added' and val in self.inputs._outputs.keys():
Copy link
Contributor Author

@oesteban oesteban Feb 10, 2015

Choose a reason for hiding this comment

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

Hi @satra, I'm not very happy with this way of checking that 'trait_added' is not interpreted as one more output. Is there a way to check this more cleanly?. If not, this PR should be merged as it fixes the bad behavior in case there are Undefined inputs.

Copy link
Contributor Author

@oesteban oesteban Feb 11, 2015

Choose a reason for hiding this comment

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

This situation also happens now in #1047

Copy link
Member

@satra satra Feb 11, 2015

Choose a reason for hiding this comment

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

i haven't looked into that in a while. it's possible to use some combination of np.setdiff1d with copyable_trait_names.

Copy link
Contributor Author

@oesteban oesteban Feb 13, 2015

Choose a reason for hiding this comment

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

I've written the same code using copyable_trait_names, but it is still necessary to check that the key is not 'trait_added'

continue

if isinstance(val, list):
for i,v in enumerate(val):
input_dict['%s_%d' % (key,i)]=v
for i, v in enumerate(val):
input_dict['%s_%d' % (key, i)] = v
else:
input_dict[key] = val

Expand Down