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

Projects
None yet
5 participants
@arokem
Member

arokem commented Mar 29, 2017

undoes changes introduced in: b6fc887

Should resolve #1107

@coveralls

This comment has been minimized.

coveralls commented Mar 29, 2017

Coverage Status

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

@codecov-io

This comment has been minimized.

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

This comment has been minimized.

Contributor

MarcCote commented Mar 30, 2017

This looks good. I can merge it later today.

@MarcCote MarcCote merged commit 69c7093 into nipy:master Mar 30, 2017

4 checks passed

codecov/patch 100% of diff hit (target 85.96%)
Details
codecov/project 85.96% (+0%) compared to acee6d7
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 88.455%
Details
@dimrozakis

This comment has been minimized.

Contributor

dimrozakis commented Mar 30, 2017

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

This comment has been minimized.

Member

arokem commented Mar 30, 2017

@dimrozakis

This comment has been minimized.

Contributor

dimrozakis commented Mar 30, 2017

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

This comment has been minimized.

Contributor

dimrozakis commented Mar 30, 2017

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

This comment has been minimized.

Member

arokem commented Mar 30, 2017

@dimrozakis

This comment has been minimized.

Contributor

dimrozakis commented Mar 30, 2017

MarcCote added a commit that referenced this pull request Apr 3, 2017

Merge pull request #1206 from arokem/revert-1204
Revert #1204, and add a filter to suppress warnings.

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

Merge pull request nipy#1204 from arokem/affine-sp-18
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

Merge pull request nipy#1206 from arokem/revert-1204
Revert nipy#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