Rebased version of JoinNode PR

[satra]

@FredLoney this replaces #643

i have rebased this on the current master.

[satra]

@mwaskom @chrisfilo - could you please review this as well?

[mwaskom]

Are you sure the rebase worked? It looks like there are some random unrelated commits in the PR still

[satra]

yes - i kept some of fred's commits that were unrelated.

[mwaskom]

This is confusing, as nib is the usual short import name for nibabel. Could cause problems for people trying to expand the tests down the line.

[satra]

this is an easy fix, but that import existed in nipype before nibabel was even around. also except for numpy and scipy i personally don't think any other short names really exist. i wouldn't worry about this one. if there is a conflict a test will catch that one.

[mwaskom]

Does it make sense to make a larger edit to the docs such that the two chapters are

• MapNode
• Iterables and JoinNode

Iterables are much more closely related to JoinNode than they are to MapNode.

[satra]

+1 for a single chapter explaining all the graph surgical procedures - but perhaps we could do that after the merge?

[mwaskom]

By the way, in rst you don't need this if you end the previous sentence with a double colon, e.g.

The code to achieve this is as folows:: import nipype... 

[FredLoney]

Hi Michael,

Thanks for the comments. I adhered as closely as possible to the layout, idioms and examples of the pre-existing Nipype manual.

The idiom of :: on a separate line is taken from there. In this and other Nipype manual conventions, I defer to the judgement of the Nipype team.

Fred

From: Michael Waskom <notifications@github.commailto:notifications@github.com>
Reply-To: nipy/nipype <reply@reply.github.commailto:reply@reply.github.com>
Date: Sunday, September 15, 2013 7:47 PM
To: nipy/nipype <nipype@noreply.github.commailto:nipype@noreply.github.com>
Cc: Me <loneyf@ohsu.edumailto:loneyf@ohsu.edu>
Subject: Re: [nipype] Rebased version of JoinNode PR (#658)

In doc/users/joinnode_and_itersource.rst:

+JoinNode, joinsource and joinfield
+==================================
+
+A :class:nipype.pipeline.engine.JoinNode generalizes MapNode to
+operate in conjunction with an upstream iterable node to reassemble
+downstream results, e.g.:
+
+.. digraph:: joinnode_ex
+

• "A" -> "B1" -> "C1" -> "D";
• "A" -> "B2" -> "C2" -> "D";
• "A" -> "B3" -> "C3" -> "D";

+The code to achieve this is as follows:
+
+::

By the way, in rst you don't need this if you end the previous sentence with a double colon, e.g.

The code to achieve this is as folows::

import nipype... 

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/658/files#r6369249.

[mwaskom]

Sure, not a big deal, just something that caught my eye!

[mwaskom]

OK I have to admit that, after reading the new doc page a couple of times (and not looking too closely at the code), I don't really feel like I understand what these new tools are or why I might want to use them. I get the general point (they let you tie back together the branches of a graph that get split by using iterables), but only in a general sense.

I think the docs might benefit from some more high-level commentary and maybe some motivation by real-world examples (either just in prose or in actual code). Maybe one of the example workflows could be updated to use this new machinery? I am just worried that all of this very nice-looking hard work will be wasted if it goes over the head of the average user.

[FredLoney]

I agree about the real-world example. Every feature added was motivated by my very real workflow as well as limitations pointed out by several other user forum posts. Unfortunately, my real-world examples are too specialized for general use. Satra, you mentioned that the features will allow you to "rewrite workflows we were not able to do as a single step". Do you have a relevant example? It is in the nature of this fairly advanced feature that any example will be structurally complex. A typical scenario we face is as follows:

• MR session DICOM inputs are filtered, modified and grouped by series.
• The filtered DICOM files are merged into one 3D NiFTI file per series.
• Several NiFTI files are averaged.
• Each NiFTI file is registered against the average.
• The combination of realigned results is input for PK modeling.
• Throw into the mix saving intermediate and final results in an XNAT subject/session/series/resource hierarchy.

In this scenario, inputs can be iterable based on previous iterables, joined, transformed and rejoined.

Fred

From: Michael Waskom <notifications@github.commailto:notifications@github.com>
Reply-To: nipy/nipype <reply@reply.github.commailto:reply@reply.github.com>
Date: Monday, September 16, 2013 10:38 AM
To: nipy/nipype <nipype@noreply.github.commailto:nipype@noreply.github.com>
Cc: Me <loneyf@ohsu.edumailto:loneyf@ohsu.edu>
Subject: Re: [nipype] Rebased version of JoinNode PR (#658)

OK I have to admit that, after reading the new doc page a couple of times (and not looking too closely at the code), I don't really feel like I understand what these new tools are or why I might want to use them. I get the general point (they let you tie back together the branches of a graph that get split by using iterables), but only in a general sense.

I think the docs might benefit from some more high-level commentary and maybe some motivation by real-world examples (either just in prose or in actual code). Maybe one of the example workflows could be updated to use this new machinery? I am just worried that all of this very nice-looking hard work will be wasted if it goes over the head of the average user.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/658#issuecomment-24528838.

[mick-d]

I see many useful uses of it:

• in Diffusion Imaging it is very common to 1) take each DW image and 2) register them to the first b0 and then 3) merge all newly registered DW volumes together into a new 4D volume
• in Connectivity analyses, it is common to 1) split a parcellation in individual ROIs, 2) act on these ROIs (say for resting state fMRI compute the mean spatial signal within each of these ROIs) and then 3) recombine them in a 4D volume
• for structural tissue segmentation, one may be interested in 1) extract each tissue class (e.g. SPM new segment) 2) act on them (apply threshold, binarize, ...) 3) recombine them into a brain mask

[mwaskom]

Sure, but in each of those examples the obvious thing to me would be to use a series of MapNodes. Don't get me wrong, I'm not saying we shouldn't accept the PR or anything. This was clearly a lot of work, and the code/tests look to be high quality. I'm just giving my perspective as a potential experienced user who found the documentation for these features somewhat confusing and not making clear what this new functionality offered that I couldn't already do.

[mick-d]

Yes so far the only solution i found was to use a series of MapNodes but then there were important limitations to this:

• i couldn't use a MapNode within a MapNode of this series (because a MapNode needed to be embed in a workflow itself iterated on for this to work) whereas this would be possible with this PR
• as pointed out by Fred in this post, a series of MapNodes doesn't parallelize well

[mwaskom]

These are two good examples of things that should be made more clear in the docs!

[FredLoney]

Satra,

https://groups.google.com/forum/?fromgroups=#!topic/nipy-user/DVYn0uNGnLM describes an error fixed by the satra@8113e55 commit included in this pull request branch. This is one of the commits that were inadvertently included in the pull request. As we discussed, I had intended to submit those minor changes as separate pull requests. I can remove those commits from this join_itersource_synchronize pull request branch if you'd prefer, although it might be easier for you to merge just the one branch.

[chrisgorgo]

I agree that the documentation might need some work, but we can do it later in a separate PR - when all of us start using this new feature and get a better hang of it. Thanks for the great work @FredLoney!

[chrisgorgo]

also - could you mention this new feature in the CHANGES file?

[satra]

could we merge this?

[mwaskom]

I agree that the documentation might need some work, but we can do it later in a separate PR

Sadly, this never happened!