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

changed vertices to float64 in evenly_distributed_sphere_642.npz #1641

Merged
merged 2 commits into from Sep 18, 2018

Conversation

Projects
None yet
3 participants
@mattcieslak
Copy link
Contributor

mattcieslak commented Sep 17, 2018

The npz file for the symmetric642 sphere caused a value error when used for peaks_from_model.

ValueError: Buffer dtype mismatch, expected 'float_t' but got 'float'

I changed the datatype to float64 and now it works.

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Sep 17, 2018

Thanks for doing this! Would you mind adding a test that fails without this change, so it doesn't creep back in?

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Sep 17, 2018

Could you take a look at #1642? Shouldn't that fail due to this issue? It passes on my machine (but let's also see what Travis says...).

@mattcieslak

This comment has been minimized.

Copy link
Contributor

mattcieslak commented Sep 17, 2018

All the other npz files had float64 vertices. The only sphere with float32's that caused errors came from get_sphere('symmetric642'), which I believe is the default sphere used in DSI Studio. get_sphere('symmetric724') has always worked for me.

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Sep 17, 2018

Oh - gotcha! I was looking at the wrong sphere...

I am not arguing that we shouldn't change this, but I worry that there might be other things afoot (probably not). See: 0ba1b63

That still passes on my machine. Are you doing more than that with your PAM?

@mattcieslak

This comment has been minimized.

Copy link
Contributor

mattcieslak commented Sep 17, 2018

I'm not doing anything with the pam yet, just trying to figure out how saving data works in dipy. Working on writing a pam5 to dsi studio converter

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Sep 18, 2018

Any other comment? I wait 24h before merging this PR and #1642

Working on writing a pam5 to dsi studio converter

look at this test, maybe it can help you

@mattcieslak

This comment has been minimized.

Copy link
Contributor

mattcieslak commented Sep 18, 2018

I don't see how it could run with float32 data as soon as it gets to here https://github.com/nipy/dipy/blob/master/dipy/reconst/recspeed.pyx#L207, so I think this PR won't cause anything to break. I added some tests, too.

Thanks for the link @skoudoro!

@arokem

This comment has been minimized.

Copy link
Member

arokem commented Sep 18, 2018

That probably means that the tests for PAM are not sufficiently exercising the code. Otherwise, #1642
should've failed. But yeah, go ahead and merge this.

@skoudoro skoudoro merged commit 7bb69da into nipy:master Sep 18, 2018

3 checks passed

codecov/patch Coverage not affected when comparing 4cad756...e3648ba
Details
codecov/project 87.35% (+0.01%) compared to 4cad756
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Sep 18, 2018

Thank you @mattcieslak for this contribution 🎉

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