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

Singleton sl vals #910

Merged
merged 3 commits into from Feb 12, 2016

Conversation

Projects
None yet
4 participants
@arokem
Member

arokem commented Feb 12, 2016

The goal: value_from_volume to deal with streamlines with only one node each.

See: #909

@arokem arokem changed the title from WIP: Singleton sl vals to Singleton sl vals Feb 12, 2016

@arokem

This comment has been minimized.

Member

arokem commented Feb 12, 2016

I've removed the "WIP" from the title. I think this is ready for a review.

@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Feb 12, 2016

Just tested it, it is working great. 👍 I'll wait tomorrow morning to merge it.

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Feb 12, 2016

How can a single point even happen? Can the tracking return a single point?

@samuelstjean

This comment has been minimized.

Contributor

samuelstjean commented Feb 12, 2016

Oh guess I should read the issues before the PR next time.

@arokem

This comment has been minimized.

Member

arokem commented Feb 12, 2016

Cool @MarcCote - thanks for the quick turnaround! Keep in mind that this is doing multiple checks in every call, so if you need more speed for your use-case (sounds interesting!), we can potentially pull out the core of the algorithm to run in an "unsafe" mode (that will do crazy stuff if not used as intended...). Just a thought. Either way, this now seems to cover that use-case as well...

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Feb 12, 2016

Cool, thx Ariel and Marco for reporting.

Garyfallidis added a commit that referenced this pull request Feb 12, 2016

@Garyfallidis Garyfallidis merged commit e95e2bb into nipy:master Feb 12, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@MarcCote

This comment has been minimized.

Contributor

MarcCote commented Feb 12, 2016

@arokem thanks for the suggestion... oh, wait I just remember there is already a function in Dipy doing a similar thing. Am I missing something or values_from_coordinates is the same as @Garyfallidis's function dipy.viz.util.map_coordinates (see comments https://github.com/nipy/dipy/pull/587/files#diff-353e2ae8dea5b15c5d1e10a4e611e78eR91) ?

@arokem

This comment has been minimized.

Member

arokem commented Feb 15, 2016

Yes. I agree with you, and with the comment you made there. That definitely doesn't belong in viz.

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented Feb 16, 2016

There is a previous discussion about moving all interpolation functions in dipy.core.interpolation or at least call them from there until the old ones get deprecated. I think it is a good plan. We should put it rolling a bit after the release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment