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

Suppress warnings regarding one-dimensional arrays changes in scipy 0.18 #1204

Merged
merged 1 commit into from Mar 30, 2017

Conversation

arokem
Copy link
Contributor

@arokem arokem commented Mar 29, 2017

undoes changes introduced in: b6fc887

Should resolve #1107

@coveralls
Copy link

Coverage Status

Coverage remained the same at 88.455% when pulling c903834 on arokem:affine-sp-18 into acee6d7 on nipy:master.

@codecov-io
Copy link

codecov-io commented Mar 29, 2017

Codecov Report

Merging #1204 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1204   +/-   ##
=======================================
  Coverage   85.96%   85.96%           
=======================================
  Files         221      221           
  Lines       27073    27073           
  Branches     2770     2770           
=======================================
  Hits        23273    23273           
  Misses       3123     3123           
  Partials      677      677
Impacted Files Coverage Δ
dipy/align/reslice.py 97.05% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update acee6d7...c903834. Read the comment docs.

@MarcCote
Copy link
Contributor

This looks good. I can merge it later today.

@MarcCote MarcCote merged commit 69c7093 into dipy:master Mar 30, 2017
@dimrozakis
Copy link
Contributor

Hey @arokem & @MarcCote

I don't think that merging this MR was the right way to go. It effectively reverts b6fc887 which had reduced completion time by about a half.

I also explained this in #1107 (comment)

I'm not sure I understand what the problem was. Scipy's docs clearly state that it is valid to supply a 1D array as the matrix argument:

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.

It seems wrong to me to revert the use of a well documented and supported feature of affine_transform and double completion time, just to get rid of a warning. Isn't it possible to just actually suppress it (as the title of this MR suggests) instead?

@arokem
Copy link
Contributor Author

arokem commented Mar 30, 2017 via email

@dimrozakis
Copy link
Contributor

Hey @arokem. Thanks for the quick response!

Both versions of calculation mentioned in scipy's docs, np.dot(matrix,o) + offset and matrix * (o + offset) yield the same result for a diagonal matrix when offset is 0. I agree with @jchoude in that:

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.

So perhaps this PR should be reverted, and replaced with a comment on the relevant code warning anyone from messing with offset in the future?

@dimrozakis
Copy link
Contributor

The problem with that warning was that it suggested that we are doing something wrong. I still don't understand whether we are, but I think that you are saying that it's all fine.

I think the warning was put there by scipy's devs to notify users of a change in the behavior of a parameter we don't use. As far as I can tell, we're not doing anything wrong here.

arokem added a commit to arokem/dipy that referenced this pull request Mar 30, 2017
@arokem
Copy link
Contributor Author

arokem commented Mar 30, 2017 via email

@dimrozakis
Copy link
Contributor

dimrozakis commented Mar 30, 2017 via email

MarcCote added a commit that referenced this pull request Apr 3, 2017
Revert #1204, and add a filter to suppress warnings.
ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018
Suppress warnings regarding one-dimensional arrays changes in scipy 0.18
ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018
ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018
Revert dipy#1204, and add a filter to suppress warnings.
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.

Dipy.align.reslice: either swallow the scipy warning or rework to avoid it
5 participants