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

Return a generator from `orient_by_roi` #1079

Merged
merged 4 commits into from Jun 28, 2016

Conversation

Projects
None yet
4 participants
@arokem
Member

arokem commented Jun 15, 2016

This is more consistent with other API for handling streamlines, and also makes more sense.

arokem added some commits Jun 15, 2016

RF: Returning a generator instead of a streamline makes more sense
And is more consistent with other API for handling streamlines.
@coveralls

This comment has been minimized.

coveralls commented Jun 15, 2016

Coverage Status

Coverage remained the same at 82.864% when pulling cab4939 on arokem:generate-orient-sl into 7c236b6 on nipy:master.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Jun 16, 2016

So you cannot orient the streamlines in-place anymore? If so, do you still need the copy parameter?

@arokem

This comment has been minimized.

Member

arokem commented Jun 16, 2016

Good point. What do you think? Should I implement a separate logic for in-place reorienting, in case the input is a list? We can make this function a little bit more flexible, I think. Currently, if you pass this function a generator, it chokes because you can't copy a generator. I stumbled on that during my work with this function, which is why I implemented this change.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Jun 16, 2016

Maybe it could have an out parameter (like Numpy's dot) instead of copy. In addition, it could have a as_generator parameter.

orient_by_rois(streamlines, roi1, roi2)  # returns a copy
orient_by_rois(streamlines, roi1, roi2, out=streamlines)  # in-place
orient_by_rois(streamlines, roi1, roi2, as_generator=True)  # returns a generator
orient_by_rois(gen_streamlines, roi1, roi2)  # returns a generator?
orient_by_rois(gen_streamlines, roi1, roi2, as_generator=True)  # returns a generator
orient_by_rois(gen_streamlines, roi1, roi2, out=gen_streamlines)  # will raise
@arokem

This comment has been minimized.

Member

arokem commented Jun 17, 2016

Yes, I like that. I'll ping again (or it seems that GH will send you an
email), when I have that implemented.

On Thu, Jun 16, 2016 at 11:45 AM, Marc-Alexandre Côté <
notifications@github.com> wrote:

Maybe it could have an out parameter (like Numpy's dot
http://docs.scipy.org/doc/numpy/reference/generated/numpy.dot.html)
instead of copy. In addition, it could have a as_generator parameter.

orient_by_rois(streamlines, roi1, roi2) # returns a copy
orient_by_rois(streamlines, roi1, roi2, out=streamlines) # in-place
orient_by_rois(streamlines, roi1, roi2, as_generator=True) # returns a generator
orient_by_rois(gen_streamlines, roi1, roi2) # returns a generator?

I'd say "yes" -- return a generator in this case. That is, keep the output
consistent with the input.

orient_by_rois(gen_streamlines, roi1, roi2, as_generator=True) # returns a generator
orient_by_rois(gen_streamlines, roi1, roi2, out=gen_streamlines) # will raise


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#1079 (comment), or mute
the thread
https://github.com/notifications/unsubscribe/AAHPNqc3xx_k9SxWQ0lehO6GI3b1yG9Bks5qMZnMgaJpZM4I2sIz
.

RF: Implement several different use-cases for orient_by_rois.
- Includes the option to return a generator or a list, as well as option
for returning a copy, or orienting in-place.
- Error handling for cases that don't make sense.
@arokem

This comment has been minimized.

Member

arokem commented Jun 27, 2016

OK - I think I got all the cases you outlined. Does the documentation seem sufficient?

@coveralls

This comment has been minimized.

coveralls commented Jun 27, 2016

Coverage Status

Coverage increased (+0.04%) to 82.908% when pulling 4cf327e on arokem:generate-orient-sl into 7c236b6 on nipy:master.

@codecov-io

This comment has been minimized.

codecov-io commented Jun 27, 2016

Current coverage is 80.38%

No coverage report found for master at 7c236b6.

Powered by Codecov. Last updated by 7c236b6...f1f4206

Helper function to `orient_by_rois`
Performs the inner loop separately. This is needed, because functions with
`yield` always return a generator.

This comment has been minimized.

@MarcCote

MarcCote Jun 28, 2016

Contributor

Maybe mention this function flips streamlines in-place and returns a reference to the updated list?

min2 = np.argmin(dist2, 0)
if min1[0] > min2[0]:
yield sl[::-1]
else:

This comment has been minimized.

@MarcCote

MarcCote Jun 28, 2016

Contributor

This else statement could be removed and its content could be dedented.

# Make a copy, so you don't change the output in place:
out = deepcopy(streamlines)
else:
out = streamlines

This comment has been minimized.

@MarcCote

MarcCote Jun 28, 2016

Contributor

From what I see, a user can provide out but it is always overwritten. Am I missing something?

This comment has been minimized.

@arokem

arokem Jun 28, 2016

Member

On further thought, I think that an out kwarg doesn't make sense. It would have to be a list of exactly the same length as the original streamlines. May as well have an inplace kwarg that would just do that.

ll = list(new_streamlines)
npt.assert_equal(ll, flipped_sl)

# Generator input with an `out` set to something raises an error:

This comment has been minimized.

@MarcCote

MarcCote Jun 28, 2016

Contributor

Generator input with an out set to something should not raise an error. However, as_generator=True and out set to something should raise an error. I think that is what you are already doing in the code anyway.

@arokem

This comment has been minimized.

Member

arokem commented Jun 28, 2016

Thanks for all the comments! I made another pass through, implementing the in_place kwarg.

@coveralls

This comment has been minimized.

coveralls commented Jun 28, 2016

Coverage Status

Coverage increased (+0.04%) to 82.908% when pulling f1f4206 on arokem:generate-orient-sl into 7c236b6 on nipy:master.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Jun 28, 2016

LGTM

@MarcCote MarcCote merged commit 3d17120 into nipy:master Jun 28, 2016

4 checks passed

codecov/patch 100% of diff hit (target 80.84%)
Details
codecov/project 80.84% (+0.03%) compared to 220d883
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.04%) to 82.908%
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment