Skip to content

FIX: Node __repr__ and detailed graph expansion #2669

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 7 commits into from
Aug 7, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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: 13 additions & 2 deletions nipype/pipeline/engine/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,11 @@ def __init__(self, name=None, base_dir=None):

"""
self._hierarchy = None
self._name = None
self.name = name
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this break the property definition below? (line 46)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

why? Before we had two lines: the first one self._name = None and 3 lines later self.name = name. I thought that one was left there forgotten.

Copy link
Member

Choose a reason for hiding this comment

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

This change looks reasonable to me. There's no instance variable used by name.setter that is set between lines 39 and 43, and nothing in these lines uses self._name in any way, so it's hard to see where this would cause a problem.

I'm not sure what @oesteban means by "break[ing] the property definition". Perhaps you could clarify?

self._id = self.name # for compatibility with node expansion using iterables

self.base_dir = base_dir
self.config = deepcopy(config._sections)
self.name = name

@property
def name(self):
Expand All @@ -66,6 +66,14 @@ def inputs(self):
def outputs(self):
raise NotImplementedError

@property
def itername(self):
"""Name for expanded iterable"""
itername = self._id
if self._hierarchy:
itername = '%s.%s' % (self._hierarchy, self._id)
return itername

def clone(self, name):
"""Clone an EngineBase object

Expand Down Expand Up @@ -95,6 +103,9 @@ def _check_inputs(self, parameter):
def __str__(self):
return self.fullname

def __repr__(self):
return self.itername

def save(self, filename=None):
if filename is None:
filename = 'temp.pklz'
Expand Down
9 changes: 0 additions & 9 deletions nipype/pipeline/engine/nodes.py
Original file line number Diff line number Diff line change
Expand Up @@ -159,7 +159,6 @@ def __init__(self,
self._got_inputs = False
self._originputs = None
self._output_dir = None
self._id = self.name # for compatibility with node expansion using iterables

self.iterables = iterables
self.synchronize = synchronize
Expand Down Expand Up @@ -249,14 +248,6 @@ def n_procs(self, value):
if hasattr(self._interface.inputs, 'num_threads'):
self._interface.inputs.num_threads = self._n_procs

@property
def itername(self):
"""Name for expanded iterable"""
itername = self._id
if self._hierarchy:
itername = '%s.%s' % (self._hierarchy, self._id)
return itername

def output_dir(self):
"""Return the location of the output directory for the node"""
# Output dir is cached
Expand Down
159 changes: 159 additions & 0 deletions nipype/pipeline/engine/tests/test_engine.py
Original file line number Diff line number Diff line change
Expand Up @@ -441,6 +441,7 @@ def test_write_graph_runs(tmpdir):

assert os.path.exists('graph.dot') or os.path.exists(
'graph_detailed.dot')

try:
os.remove('graph.dot')
except OSError:
Expand Down Expand Up @@ -484,6 +485,164 @@ def test_deep_nested_write_graph_runs(tmpdir):
pass


# examples of dot files used in the following test
dotfile_orig = ['strict digraph {\n',
'"mod1 (engine)";\n',
'"mod2 (engine)";\n',
'"mod1 (engine)" -> "mod2 (engine)";\n',
'}\n']

dotfile_detailed_orig = ['digraph structs {\n',
'node [shape=record];\n',
'pipemod1 [label="{IN}|{ mod1 | engine | }|{OUT|<outoutput1> output1}"];\n',
'pipemod2 [label="{IN|<ininput1> input1}|{ mod2 | engine | }|{OUT}"];\n',
'pipemod1:outoutput1:e -> pipemod2:ininput1:w;\n',
'}']


dotfile_hierarchical = ['digraph pipe{\n',
' label="pipe";\n',
' pipe_mod1[label="mod1 (engine)"];\n',
' pipe_mod2[label="mod2 (engine)"];\n',
' pipe_mod1 -> pipe_mod2;\n',
'}']

dotfile_colored = ['digraph pipe{\n',
' label="pipe";\n',
' pipe_mod1[label="mod1 (engine)", style=filled, fillcolor="#FFFFC8"];\n',
' pipe_mod2[label="mod2 (engine)", style=filled, fillcolor="#FFFFC8"];\n',
' pipe_mod1 -> pipe_mod2;\n',
'}']

dotfiles = {
"orig": dotfile_orig,
"flat": dotfile_orig,
"exec": dotfile_orig,
"hierarchical": dotfile_hierarchical,
"colored": dotfile_colored
}

@pytest.mark.parametrize("simple", [True, False])
@pytest.mark.parametrize("graph_type", ['orig', 'flat', 'exec', 'hierarchical', 'colored'])
def test_write_graph_dotfile(tmpdir, graph_type, simple):
""" checking dot files for a workflow without iterables"""
tmpdir.chdir()

pipe = pe.Workflow(name='pipe')
mod1 = pe.Node(interface=EngineTestInterface(), name='mod1')
mod2 = pe.Node(interface=EngineTestInterface(), name='mod2')
pipe.connect([(mod1, mod2, [('output1', 'input1')])])
pipe.write_graph(
graph2use=graph_type, simple_form=simple, format='dot')

with open("graph.dot") as f:
graph_str = f.read()

if simple:
for line in dotfiles[graph_type]:
assert line in graph_str
else:
# if simple=False graph.dot uses longer names
for line in dotfiles[graph_type]:
if graph_type in ["hierarchical", "colored"]:
assert line.replace("mod1 (engine)", "mod1.EngineTestInterface.engine").replace(
"mod2 (engine)", "mod2.EngineTestInterface.engine") in graph_str
else:
assert line.replace(
"mod1 (engine)", "pipe.mod1.EngineTestInterface.engine").replace(
"mod2 (engine)", "pipe.mod2.EngineTestInterface.engine") in graph_str

# graph_detailed is the same for orig, flat, exec (if no iterables)
# graph_detailed is not created for hierachical or colored
if graph_type not in ["hierarchical", "colored"]:
with open("graph_detailed.dot") as f:
graph_str = f.read()
for line in dotfile_detailed_orig:
assert line in graph_str


# examples of dot files used in the following test
dotfile_detailed_iter_exec = [
'digraph structs {\n',
'node [shape=record];\n',
'pipemod1aIa1 [label="{IN}|{ a1 | engine | mod1.aI }|{OUT|<outoutput1> output1}"];\n',
'pipemod2a1 [label="{IN|<ininput1> input1}|{ a1 | engine | mod2 }|{OUT}"];\n',
'pipemod1aIa0 [label="{IN}|{ a0 | engine | mod1.aI }|{OUT|<outoutput1> output1}"];\n',
'pipemod2a0 [label="{IN|<ininput1> input1}|{ a0 | engine | mod2 }|{OUT}"];\n',
'pipemod1aIa0:outoutput1:e -> pipemod2a0:ininput1:w;\n',
'pipemod1aIa1:outoutput1:e -> pipemod2a1:ininput1:w;\n',
'}']

dotfile_iter_hierarchical = [
'digraph pipe{\n',
' label="pipe";\n',
' pipe_mod1[label="mod1 (engine)", shape=box3d,style=filled, color=black, colorscheme=greys7 fillcolor=2];\n',
' pipe_mod2[label="mod2 (engine)"];\n',
' pipe_mod1 -> pipe_mod2;\n',
'}']

dotfile_iter_colored = [
'digraph pipe{\n',
' label="pipe";\n',
' pipe_mod1[label="mod1 (engine)", shape=box3d,style=filled, color=black, colorscheme=greys7 fillcolor=2];\n',
' pipe_mod2[label="mod2 (engine)", style=filled, fillcolor="#FFFFC8"];\n',
' pipe_mod1 -> pipe_mod2;\n',
'}']

dotfiles_iter = {
"orig": dotfile_orig,
"flat": dotfile_orig,
"exec": dotfile_orig,
"hierarchical": dotfile_iter_hierarchical,
"colored": dotfile_iter_colored
}

dotfiles_detailed_iter = {
"orig": dotfile_detailed_orig,
"flat": dotfile_detailed_orig,
"exec": dotfile_detailed_iter_exec
}

@pytest.mark.parametrize("simple", [True, False])
@pytest.mark.parametrize("graph_type", ['orig', 'flat', 'exec', 'hierarchical', 'colored'])
def test_write_graph_dotfile_iterables(tmpdir, graph_type, simple):
""" checking dot files for a workflow with iterables"""
tmpdir.chdir()

pipe = pe.Workflow(name='pipe')
mod1 = pe.Node(interface=EngineTestInterface(), name='mod1')
mod1.iterables = ('input1', [1, 2])
mod2 = pe.Node(interface=EngineTestInterface(), name='mod2')
pipe.connect([(mod1, mod2, [('output1', 'input1')])])
pipe.write_graph(
graph2use=graph_type, simple_form=simple, format='dot')

with open("graph.dot") as f:
graph_str = f.read()

if simple:
for line in dotfiles_iter[graph_type]:
assert line in graph_str
else:
# if simple=False graph.dot uses longer names
for line in dotfiles_iter[graph_type]:
if graph_type in ["hierarchical", "colored"]:
assert line.replace("mod1 (engine)", "mod1.EngineTestInterface.engine").replace(
"mod2 (engine)", "mod2.EngineTestInterface.engine") in graph_str
else:
assert line.replace(
"mod1 (engine)", "pipe.mod1.EngineTestInterface.engine").replace(
"mod2 (engine)", "pipe.mod2.EngineTestInterface.engine") in graph_str

# graph_detailed is not created for hierachical or colored
if graph_type not in ["hierarchical", "colored"]:
with open("graph_detailed.dot") as f:
graph_str = f.read()
for line in dotfiles_detailed_iter[graph_type]:
assert line in graph_str



def test_io_subclass():
"""Ensure any io subclass allows dynamic traits"""
from nipype.interfaces.io import IOBase
Expand Down
6 changes: 3 additions & 3 deletions nipype/pipeline/engine/utils.py
Original file line number Diff line number Diff line change
Expand Up @@ -507,7 +507,7 @@ def _write_detailed_dot(graph, dotfilename):
# write nodes
edges = []
for n in nx.topological_sort(graph):
nodename = str(n)
nodename = n.itername
inports = []
for u, v, d in graph.in_edges(nbunch=n, data=True):
for cd in d['connect']:
Expand All @@ -519,8 +519,8 @@ def _write_detailed_dot(graph, dotfilename):
ipstrip = 'in%s' % _replacefunk(inport)
opstrip = 'out%s' % _replacefunk(outport)
edges.append(
'%s:%s:e -> %s:%s:w;' % (str(u).replace('.', ''), opstrip,
str(v).replace('.', ''), ipstrip))
'%s:%s:e -> %s:%s:w;' % (u.itername.replace('.', ''), opstrip,
v.itername.replace('.', ''), ipstrip))
if inport not in inports:
inports.append(inport)
inputstr = ['{IN'] + [
Expand Down