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

Dipy.align.reslice: either swallow the scipy warning or rework to avoid it #1107

Closed
jchoude opened this issue Aug 8, 2016 · 6 comments · Fixed by #1204
Closed

Dipy.align.reslice: either swallow the scipy warning or rework to avoid it #1107

jchoude opened this issue Aug 8, 2016 · 6 comments · Fixed by #1204

Comments

@jchoude
Copy link
Contributor

jchoude commented Aug 8, 2016

When calling dipy.align.reslice and use scipy 0.17.1, we get the following UserWarning:

scipy/ndimage/interpolation.py:435: UserWarning: The behaviour of affine_transform with a one-dimensional array supplied for the matrix parameter will change in scipy 0.18.0.
  "The behaviour of affine_transform with a one-dimensional "

This is caused by a change in behavior from Scipy, between 0.17.1 and 0.18, as explained in Scipy's doc.

For us, there won't be any consequence, since the behavior won't change, because we do not use the offset parameter. However, the warning might be misleading or simply distracting for end users, and it will still be present in scipy 0.18. That means that it will pollute our logs for a long time.

Two ideas:
1- Swallow the warning by using something like this (as was suggested in issue #641 )
2- Simply reformat the matrix we send as a diagonal matrix, which will provide exactly the same behavior, without the warning.

I would go for option 2, but would like to have some other advice before proceeding @Garyfallidis @arokem

@arokem
Copy link
Contributor

arokem commented Aug 10, 2016

Yes: I like option 2 better too. Presumably changing this line?

https://github.com/nipy/dipy/blob/master/dipy/align/reslice.py#L71

to the following?

R = np.diag(new_zooms / zooms)

@quantshah
Copy link
Contributor

Changing to R = np.diag(new_zooms / zooms) fails the test

FAIL: dipy.align.tests.test_reslice.test_resample
----------------------------------------------------------------------
Traceback (most recent call last):
  File "dipy/dipy/align/tests/test_reslice.py", line 25, in test_resample
    assert_almost_equal(new_zooms, new_zooms_confirmed)

AssertionError: 
Arrays are not almost equal to 7 decimals

(mismatch 100.0%)
 x: array([ 1. ,  1.2,  2.1])
 y: array([ 1.7320508,  2.0784609,  3.6373067], dtype=float32)

@quantshah
Copy link
Contributor

@arokem Any comments on this ? Is changing test parameters okay or is R = np.diag(new_zooms / zooms) not the correct way to go. @jchoude

@arokem
Copy link
Contributor

arokem commented Sep 13, 2016

Yes. I must have gotten that wrong. @jchoude : what would you suggest here?

@dimrozakis
Copy link
Contributor

Hey @arokem & @sahmed95.

R used to be a diagonal array. I changed it to a single dimensional array in b6fc887. The reason for that, as explained in the commit message and in scipy's docs is:

A diagonal matrix can be specified by supplying a one-dimensional array-like to the matrix parameter, in which case a more efficient algorithm is applied.

So I would suggest against changing it back to a 2D array, since that will increase computation time of reslice.

@jchoude
Copy link
Contributor Author

jchoude commented Oct 26, 2016

Sorry for the delay.

I don't mind leaving it as a 1D array. However, what was not clear to me was the potential difference in behavior between Scipy 0.17.1 and 0.18 when supplying a 1D array, exactly in the same doc that @dimrozakis pointed out.

Currently, our behavior is consistent, since the offset that we pass is always np.zeros(3,). We might simply need to ensure that we don't change the offset, else the result might not be consistent between versions of scipy.

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 a pull request may close this issue.

4 participants