FIX: Check against full node name when reconnecting JoinNodes

[effigies]

Fixes #2257.

Changes proposed in this pull request

• Add test to replicate Issue with node name that starts with another node's name #2257.
• Apply fix suggested by @williford.

[satra]

@effigies - this is same as https://github.com/nipy/nipype/pull/1708/files - i'll have to recall all the scenarios where it would break. but hopefully the tests will catch some of them.

i think it has to do with the fact that nipype node id's are hierarchical. a.b.c. ..., so just checking by adding . is insufficient i believe.

[effigies]

Ah, missed that. Was also counting on the tests to verify.

[effigies]

Yes, a valid match either is exactly the node, or has a .aN (through .zN). I pushed a fix that uses a full regular expression src_id + r'([a-z]\d+)?$'. I think this should exhaust the cases that can actually happen. If you can think of a good corner case that might trip us up if we fail, I can add another test.

[effigies]

Okay. I printed out the node type and the names being compared every time we hit that case. It turns out that having IdentityInterfaces as either the iterable Node or the JoinNode had different consequences on the names, so there are a few cases to consider. I think I covered them all. Certainly all of the ones provided by the test suite, though I'll note that the test I added is the first one not to use IdentityInterface as the joinsource.

[satra]

@effigies - the matching looks clean. thanks for pushing this along.

[tclose]

The change

if hasattr(node, 'joinfield'):
    continue

has broken my my pipeline generation code but I don't understand the logic well enough to isolate the problem (my code is quite complex due to it being a general framework). What is the rationale behind this check?

My first thought was that it could be due to the fact that the iterable that the join node joins can be of length one, but when I tried that in the unittest you added I couldn't reproduce it.

[effigies]

Hi @tclose, sorry about that. Could you open an issue referencing this PR? I completely forgot about your comment here, since closed PRs don't show up as active issues.

[tclose]

Hi Chris, Sorry for the delay in responding. I had a few goes at creating a MWE that didn't involve my package but failed so have just included one that does. I have created an issue for it here <#2578>. Many thanks, Tom

…

On 3 April 2018 at 00:19, Chris Markiewicz ***@***.***> wrote: Hi @tclose <https://github.com/tclose>, sorry about that. Could you open an issue <https://github.com/nipy/nipype/issues> referencing this PR? I completely forgot about your comment here, since closed PRs don't show up as active issues. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2479 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABAYlqiXRt_Ir1JvrIVIamWpVTflKn6Rks5tkjNpgaJpZM4SZF24> .

-- *THOMAS G. CLOSE, PHD* Senior Informatics Officer *Monash Biomedical Imaging* Monash University Room 139, 770 Blackburn Rd Clayton Campus, Clayton VIC 3800 Australia T: +61 3 9902 9804 M: +61 491 141 390 E: tom.close@monash.edu mbi.monash.edu.au