Skip to content

ENH: Make interfaces.utility.Merge(1) merge a list of lists #1390

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 4 commits into from
Mar 22, 2017

Conversation

effigies
Copy link
Member

@effigies effigies commented Mar 3, 2016

At present, Merge() does nothing, providing an undefined outputs.out. This change allows it to merge a list of lists (inputs.in_lists) into a single list, rather than enumerating them with inputs.in1 .. inputs.inN.

If no inputs.in_lists is set, Merge() still provides an undefined outputs.out, so we shouldn't break existing code.

Merge(n) for n < 0 is unchanged.

effigies added a commit to effigies/nipype that referenced this pull request Mar 3, 2016
effigies added a commit to effigies/nipype that referenced this pull request Mar 5, 2016
effigies added a commit to effigies/nipype that referenced this pull request Mar 21, 2016
@coveralls
Copy link

Coverage Status

Coverage increased (+0.02%) to 72.998% when pulling 88f4df5 on effigies:merge_all into d5497c9 on nipy:master.

effigies added a commit to effigies/nipype that referenced this pull request Mar 22, 2016
@effigies
Copy link
Member Author

Slight amendment: Merge(0, axis='hstack') previously failed to run. It now behaves the same as Merge(0, axis='vstack'), if no in_lists is provided.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.04%) to 73.02% when pulling 46bc838 on effigies:merge_all into c46b932 on nipy:master.

effigies added a commit to effigies/nipype that referenced this pull request Apr 6, 2016
effigies added a commit to effigies/nipype that referenced this pull request Apr 7, 2016
effigies added a commit to effigies/nipype that referenced this pull request Jul 13, 2016
@coveralls
Copy link

coveralls commented Jul 13, 2016

Coverage Status

Coverage increased (+0.03%) to 72.321% when pulling 30fbbe7 on effigies:merge_all into 6d8efd7 on nipy:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 72.321% when pulling 30fbbe7 on effigies:merge_all into 6d8efd7 on nipy:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.03%) to 72.321% when pulling 30fbbe7 on effigies:merge_all into 6d8efd7 on nipy:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.002%) to 72.289% when pulling 30fbbe7 on effigies:merge_all into 6d8efd7 on nipy:master.

@coveralls
Copy link

coveralls commented Jul 13, 2016

Coverage Status

Coverage increased (+0.03%) to 72.321% when pulling 30fbbe7 on effigies:merge_all into 6d8efd7 on nipy:master.

effigies added a commit to effigies/nipype that referenced this pull request Aug 8, 2016
@coveralls
Copy link

coveralls commented Aug 8, 2016

Coverage Status

Changes Unknown when pulling 9c88205 on effigies:merge_all into * on nipy:master*.

effigies added a commit to effigies/nipype that referenced this pull request Aug 16, 2016
@coveralls
Copy link

coveralls commented Aug 16, 2016

Coverage Status

Changes Unknown when pulling 9ec3985 on effigies:merge_all into * on nipy:master*.

effigies added a commit to effigies/nipype that referenced this pull request Aug 21, 2016
@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 72.307% when pulling ea19acc on effigies:merge_all into 304ffa4 on nipy:master.

3 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 72.307% when pulling ea19acc on effigies:merge_all into 304ffa4 on nipy:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.07%) to 72.307% when pulling ea19acc on effigies:merge_all into 304ffa4 on nipy:master.

@coveralls
Copy link

coveralls commented Aug 21, 2016

Coverage Status

Coverage increased (+0.07%) to 72.307% when pulling ea19acc on effigies:merge_all into 304ffa4 on nipy:master.

effigies added a commit to effigies/nipype that referenced this pull request Dec 22, 2016
effigies added a commit to effigies/nipype that referenced this pull request Dec 22, 2016
@codecov-io
Copy link

codecov-io commented Dec 22, 2016

Codecov Report

Merging #1390 into master will increase coverage by 0.02%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master    #1390      +/-   ##
==========================================
+ Coverage   70.91%   70.93%   +0.02%     
==========================================
  Files        1057     1057              
  Lines       53363    53383      +20     
  Branches     7728     7736       +8     
==========================================
+ Hits        37843    37868      +25     
+ Misses      14139    14137       -2     
+ Partials     1381     1378       -3
Flag Coverage Δ
#smoketests 70.93% <100%> (+0.02%) ⬆️
#unittests 70.38% <100%> (+0.02%) ⬆️
Impacted Files Coverage Δ
nipype/interfaces/utility/tests/test_base.py 100% <100%> (ø) ⬆️
nipype/interfaces/utility/base.py 89.47% <100%> (+4.62%) ⬆️
nipype/interfaces/base.py 83.28% <0%> (-0.19%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 30ab0e1...fcf1d48. Read the comment docs.

effigies added a commit to effigies/nipype that referenced this pull request Dec 30, 2016
@effigies effigies force-pushed the merge_all branch 2 times, most recently from 12be718 to e933eb2 Compare March 22, 2017 02:15
input_names = ['in%d' % (i + 1) for i in range(numinputs)]
elif numinputs == 0:
input_names = ['in_lists']
else:
Copy link
Member

Choose a reason for hiding this comment

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

so the only question here is whether this should be for Merge(1) instead of Merge(0)

Copy link
Member Author

Choose a reason for hiding this comment

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

I can do Merge(1). The goal was to leave the interface unmodified for any existing uses, but I guess nobody would actually do Merge(1) as it stands.

If we do, I'd suggest making numinputs default to 1, so that Merge() can be useful.

And I can update the doctests to be demonstrative.

@satra
Copy link
Member

satra commented Mar 22, 2017

sounds good.

If accumulated list is empty, return as valid, rather than undefined
@effigies effigies changed the title ENH: Make interfaces.utility.Merge(0) merge a list of lists ENH: Make interfaces.utility.Merge(1) merge a list of lists Mar 22, 2017
@oesteban oesteban merged commit 2e211f7 into nipy:master Mar 22, 2017
@effigies effigies deleted the merge_all branch March 22, 2017 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants