Skip to content

Add a function to concatenate multiple ArraySequence objects #494

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

Jump to bottom
Merged
merged 5 commits into from
Feb 15, 2017
Merged

Add a function to concatenate multiple ArraySequence objects #494

merged 5 commits into from
Feb 15, 2017

Conversation

MarcCote
Copy link
Contributor

@MarcCote MarcCote commented Nov 7, 2016

This PR adds the function nibabel.streamlines.array_sequence.concatenate(seqs, axis) that can concatenate multiple ArraySequence objects along the provided axis.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 95.951% when pulling f788cb9 on MarcCote:enh_concatenate_array_sequences into ec4567f on nipy:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 95.951% when pulling 261bf3b on MarcCote:enh_concatenate_array_sequences into ec4567f on nipy:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 95.951% when pulling 4107401 on MarcCote:enh_concatenate_array_sequences into ec4567f on nipy:master.

@codecov-io
Copy link

codecov-io commented Nov 7, 2016 •
edited
Loading

Codecov Report

Merging #494 into master will increase coverage by 0.06%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #494      +/-   ##
==========================================
+ Coverage   93.97%   94.03%   +0.06%     
==========================================
  Files         166      166              
  Lines       21836    22015     +179     
  Branches     2327     2345      +18     
==========================================
+ Hits        20520    20702     +182     
+ Misses        885      878       -7     
- Partials      431      435       +4
Impacted Files Coverage Δ
nibabel/streamlines/tests/test_array_sequence.py 99.49% <100%> (+0.03%) ✅
nibabel/streamlines/array_sequence.py 98.91% <100%> (+0.04%) ✅
nibabel/streamlines/tests/test_tractogram.py 97.66% <ø> (-1.32%) ❌
nibabel/viewers.py 96.66% <ø> (-0.67%) ❌
nibabel/info.py 100% <ø> (ø) ✅
nibabel/tests/test_minc1.py 96.69% <ø> (ø) ✅
nibabel/tests/test_minc2.py 95.23% <ø> (ø) ✅
nibabel/spm2analyze.py 93.75% <ø> (ø) ✅
nibabel/imageclasses.py 100% <ø> (ø) ✅
nibabel/casting.py 85.77% <ø> (ø) ✅
... and 7 more

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 ec4567f...09f5de5. Read the comment docs.

@nilgoyette
Copy link

Tested for my RGB use case (zipping 3 ArraySequence) and work perfectly.
GJ.

matthew-brett
matthew-brett reviewed Jan 12, 2017
View reviewed changes
Copy link
Member

@matthew-brett matthew-brett left a comment

Choose a reason for hiding this comment

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

A few small comments.

nibabel/streamlines/array_sequence.py Outdated

Parameters
----------
seqs: list of :class:`ArraySequence` objects
Copy link
Member

Choose a reason for hiding this comment

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

I guess it just has to be iterable, and does not have to be exactly a list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. I'll clarify.

nibabel/streamlines/tests/test_array_sequence.py


def test_concatenate():
seq = SEQ_DATA['seq'].copy() # In case there is in-place modification.
Copy link
Member

Choose a reason for hiding this comment

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

Should there be in-place modification? Worth testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, it shouldn't. I added a test.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 96.011% when pulling a3aedc7 on MarcCote:enh_concatenate_array_sequences into ec4567f on nipy:master.

matthew-brett
matthew-brett reviewed Jan 25, 2017
View reviewed changes
nibabel/streamlines/tests/test_array_sequence.py Outdated
seq = SEQ_DATA['seq']
seqs = [seq[:, [i]] for i in range(seq.common_shape[0])]
new_seq = concatenate(seqs, axis=0)
assert_true(len(new_seq), seq.common_shape[0]*len(seq))
Copy link
Member

Choose a reason for hiding this comment

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

I think PEP8 likes spaces around the * here.

@MarcCote
Copy link
Contributor Author

I totally forgot about this one. @matthew-brett: all your comments should be addressed now.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.06%) to 96.012% when pulling 09f5de5 on MarcCote:enh_concatenate_array_sequences into ec4567f on nipy:master.

@matthew-brett matthew-brett merged commit 7044ee4 into nipy:master Feb 15, 2017
@matthew-brett
Copy link
Member

Thanks - merged.

@MarcCote MarcCote deleted the enh_concatenate_array_sequences branch February 15, 2017 14:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthew-brett matthew-brett matthew-brett left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@MarcCote @coveralls @codecov-io @nilgoyette @matthew-brett