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

MAINT: work around scipy bug in sph_harm #652

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@argriffing

argriffing commented May 19, 2015

related bugs/issues/PRs:
scipy/scipy#4887
scipy/scipy#4895
#611
#612

@argriffing

This comment has been minimized.

argriffing commented May 20, 2015

I don't think the dipy travis config uses a new enough scipy to test this change.

@arokem

This comment has been minimized.

Member

arokem commented May 20, 2015

I think you are correct. Let me try this out on my laptop.

@arokem

This comment has been minimized.

Member

arokem commented May 20, 2015

I can confirm that this works on scipy 0.15 on my laptop, killing #611 and obviating #612.

@arokem

This comment has been minimized.

Member

arokem commented May 20, 2015

Another option is to wait until this is fixed upstream in scipy, and keep using our implementation until that time (which is what #612 now implements). Any thoughts from anyone else?

@arokem

This comment has been minimized.

Member

arokem commented May 20, 2015

Also: thanks so much for taking a look!

@pv

This comment has been minimized.

pv commented May 20, 2015

This is probably a numpy bug in ufunc dtype selection (which is choosing float32 instead of float64), so the upstream scipy fix will also be a workaround (and you will have to wait for it for a moderately long time as your users would need to use a new enough Scipy version). The problem is fairly restricted --- it occurs only in the special case when all arguments are integers, and at least one of the first two are arrays, and both of the angle arguments are integer scalars.

@arokem

This comment has been minimized.

Member

arokem commented May 20, 2015

Thanks for clarifying. I still think that we can wait. We do have a fully
functioning solution in our code. This particular PR seems to solve the
problem for only a small subset of the potential failure modes of this
function (only when used through sh_to_rh). I prefer the solution we have
on #612, and waiting.

Anyone else but me have any opinions on this?

On Wed, May 20, 2015 at 1:40 AM, Pauli Virtanen notifications@github.com
wrote:

This is probably a numpy bug in ufunc dtype selection (which is choosing
float32 instead of float64), so the upstream scipy fix will also be a
workaround (and you will have to wait for it for a moderately long time as
your users would need to use a new enough Scipy version). The problem is
fairly restricted --- it occurs only in the special case when all arguments
are integers, at least one of the first two are arrays, and both of the
angle arguments are integer scalars.


Reply to this email directly or view it on GitHub
#652 (comment).

@argriffing

This comment has been minimized.

argriffing commented May 20, 2015

This particular PR seems to solve the problem for only a small subset of the potential failure modes of this function (only when used through sh_to_rh)

Sorry this PR was a bit tongue-in-cheek. If you never want low-precision output from sph_harm then I think you could change
return sph_harm(m, n, theta, phi)
to
return sph_harm(m, n, theta, phi, dtype=complex)
at
https://github.com/nipy/dipy/blob/maint/0.9.x/dipy/reconst/shm.py#L189

arokem added a commit to arokem/dipy that referenced this pull request May 20, 2015

@arokem

This comment has been minimized.

Member

arokem commented May 20, 2015

Oh - you shouldn't mess with me that way. I'm way too slow to get that kind
of humor. And awfully gullible, when it comes to PRs.

Thanks for the new suggestion. This one was serious, right?

Now implemented in #612.

On Wed, May 20, 2015 at 10:22 AM, argriffing notifications@github.com
wrote:

This particular PR seems to solve the problem for only a small subset of
the potential failure modes of this function (only when used through
sh_to_rh)

Sorry this PR was a bit tongue-in-cheek. If you never want low-precision
output from sph_harm then I think you could change
return sph_harm(m, n, theta, phi)
to
return sph_harm(m, n, theta, phi, dtype=complex)
at
https://github.com/nipy/dipy/blob/maint/0.9.x/dipy/reconst/shm.py#L189


Reply to this email directly or view it on GitHub
#652 (comment).

@argriffing

This comment has been minimized.

argriffing commented May 20, 2015

Oh - you shouldn't mess with me that way. I'm way too slow to get that kind
of humor. And awfully gullible, when it comes to PRs.

Thanks for the new suggestion. This one was serious, right?

@pv Do you have a suggestion for the best practice for working around this bug in packages downstream from numpy/scipy? I think the method in this PR would work for dipy, and that the less ad-hoc workaround of adding dtype=complex in the dipy wrapper of sph_harm would also work (assuming that they never want complex64 output under any circumstance). Do you agree that the second of these two choices would be more appropriate? Is there an even better workaround?

@Garyfallidis

This comment has been minimized.

Member

Garyfallidis commented May 30, 2015

Ping! How is this PR going on? Still a work in progress?

@arokem

This comment has been minimized.

Member

arokem commented May 31, 2015

Hi! This one is probably superseded by #612. Closing this one for now.

@arokem arokem closed this May 31, 2015

arokem added a commit to arokem/dipy that referenced this pull request Jun 5, 2015

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