Skip to content
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

trilinear_interpolate4d only works on float64 #832

Closed
wants to merge 3 commits into from

Conversation

samuelstjean
Copy link
Contributor

So now it force the shm coefficient to float64. Could also patch the cython file to accept more datatype, thought maybe float32 is not precise enough, who knows.

@samuelstjean
Copy link
Contributor Author

So basically trying to feed float32 shm coeff does not work as per this traceback

Traceback (most recent call last):
  File "/home/samuel/git/scilpy/scripts/tracking.py", line 32, in <module>
    save_trk(out_track, tracker, affine, seed_mask.shape)
  File "/home/samuel/python/dipy/io/trackvis.py", line 31, in save_trk
    nib.trackvis.write(filename, data, hdr)
  File "/home/samuel/.local/lib/python2.7/site-packages/nibabel/trackvis.py", line 337, in write
    streams0 = next(stream_iter)
  File "/home/samuel/python/dipy/io/trackvis.py", line 24, in <genexpr>
    data = ((p, None, None) for p in points)
  File "/home/samuel/python/dipy/tracking/utils.py", line 916, in move_streamlines
    for sl in streamlines:
  File "/home/samuel/python/dipy/tracking/utils.py", line 916, in move_streamlines
    for sl in streamlines:
  File "/home/samuel/python/dipy/tracking/local/localtracking.py", line 108, in _generate_streamlines
    directions = dg.initial_direction(s)
  File "/home/samuel/python/dipy/direction/probabilistic_direction_getter.py", line 216, in initial_direction
    pmf = self.pmf_gen.get_pmf(point)
  File "/home/samuel/python/dipy/direction/probabilistic_direction_getter.py", line 46, in get_pmf
    coeff = trilinear_interpolate4d(self.shcoeff, point)
  File "dipy/tracking/local/interpolation.pyx", line 72, in dipy.tracking.local.interpolation.trilinear_interpolate4d (dipy/tracking/local/interpolation.c:2389)
ValueError: Buffer dtype mismatch, expected 'double' but got 'float'

@arokem
Copy link
Contributor

arokem commented Jan 11, 2016

Could you please provide a test that fails with the old behavior?

@samuelstjean
Copy link
Contributor Author

I did, look at the first commit, the second one is the fix.

2016-01-11 16:43 GMT+01:00 Ariel Rokem notifications@github.com:

Could you please provide a test that fails with the old behavior?


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

@samuelstjean
Copy link
Contributor Author

Or rather I made a test fail on purpose XD

@arokem
Copy link
Contributor

arokem commented Jan 11, 2016

Yes, I see that you did there, but this is not persistent in the code base. I would like you to add a test case that would prevent this behavior from creeping back into the code-base. That is, write a unit test in test_probabilistic_direction_getter that fails in the code as it is currently on master and passes with this fix.

@samuelstjean
Copy link
Contributor Author

I read that wrong the first time then, I'll try something a bit later.

@arokem
Copy link
Contributor

arokem commented Jan 17, 2016

Hi @samuelstjean - could you please add a test case, instead of changing the existing test-case? That is, add an additional test case, that demonstrates the bug that this is supposed to fix. Ideally (say, if you decided to start over on another branch), you would do this in two consecutive commits, allowing us to see what the problem is (what the test is, and what failure it produces on Travis), and how you propose to fix it.

@samuelstjean
Copy link
Contributor Author

That was supposed to be the bug and the fix, but pushing both did not
trigger the build, possible to do it without repushing?

I guess I can just copy paste the whole thing, but do people try different
stuff on these? I just fed it a regular output, so it would be weird nobody
noticed this before.
On Jan 17, 2016 05:31, "Ariel Rokem" notifications@github.com wrote:

Hi @samuelstjean https://github.com/samuelstjean - could you please
add a test case, instead of changing the existing test-case? That is,
add an additional test case, that demonstrates the bug that this is
supposed to fix. Ideally (say, if you decided to start over on another
branch), you would do this in two consecutive commits, allowing us to see
what the problem is (what the test is, and what failure it produces on
Travis), and how you propose to fix it.


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

@samuelstjean
Copy link
Contributor Author

So I expanded the test to run on different dtype of the same data instead, does its purpose without writing the same thing twice.

@arokem
Copy link
Contributor

arokem commented Jan 18, 2016

Yep. That's was the direction of what I was looking for. Just a bit more (see Travis errors)

@samuelstjean
Copy link
Contributor Author

I have been trying to git revert, reset --hard and a bunch of thing, will
fix it tomorrow. I always end up squashing my working version with
something random, go figure.

2016-01-18 16:24 GMT+01:00 Ariel Rokem notifications@github.com:

Yep. That's what the direction of what I was looking for. Just a bit more
(see Travis errors)


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

@arokem
Copy link
Contributor

arokem commented Jan 18, 2016

This might be one for an interactive rebase
https://www.atlassian.com/git/tutorials/rewriting-history/git-rebase-i,
but please read up on it, before you go headlong in.

On Mon, Jan 18, 2016 at 7:28 AM, Samuel St-Jean notifications@github.com
wrote:

I have been trying to git revert, reset --hard and a bunch of thing, will
fix it tomorrow. I always end up squashing my working version with
something random, go figure.

2016-01-18 16:24 GMT+01:00 Ariel Rokem notifications@github.com:

Yep. That's what the direction of what I was looking for. Just a bit more
(see Travis errors)


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


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

@arokem
Copy link
Contributor

arokem commented Feb 4, 2016

Any progress here? I think you might consider keeping the tests you have here, but using this approach instead of the cast to float:

arokem@822409f

@arokem
Copy link
Contributor

arokem commented Mar 15, 2019

Closing in favor of #1784

@arokem arokem closed this Mar 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants