Skip to content

Fix GH14900: numpy 1.17.0 breaks test_colors. #14901

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 28, 2019
Merged

Fix GH14900: numpy 1.17.0 breaks test_colors. #14901

merged 1 commit into from
Jul 28, 2019

Conversation

bingyao
Copy link
Contributor

@bingyao bingyao commented Jul 27, 2019 •
edited
Loading

PR Summary

Try to fix #14900

Part I

In the original file(colors.py), the following lines should be (functionally) equivalent:

>>> # A: using assignment and output arguments AT THE SAME TIME
>>> intensity = np.clip(intensity, 0, 1, intensity)
>>> # B: using output arguments
>>> np.clip(intensity, 0, 1, out=intensity)
>>> # C: WITHOUT using output arguments
>>> intensity = np.clip(intensity, 0, 1)

Although I know the ufunc's output argument can save memory, but I don't quite understand why the original file used A instead of B? (whether was a misusage or there is some advantage?)

Part II

Everything was fine until Numpy 1.17.0 where np.clip breaks the equivalence with MaskedArray:

In [1]: import numpy as np                                                                                                                                                 

In [2]: np.__version__                                                                                                                                                     
Out[2]: '1.17.0'

In [3]: a = np.arange(10)                                                                                                                                                  

In [4]: m = np.ma.MaskedArray(a, mask=[0, 1] * 5)                                                                                                                          

In [5]: np.clip(m, 0, 5)                                                                                                                                                   
Out[5]: 
masked_array(data=[0, --, 2, --, 4, --, 5, --, 5, --],
             mask=[False,  True, False,  True, False,  True, False,  True,
                   False,  True],
       fill_value=999999)

In [6]: m                                                                                                                                                                  
Out[6]: 
masked_array(data=[0, --, 2, --, 4, --, 6, --, 8, --],
             mask=[False,  True, False,  True, False,  True, False,  True,
                   False,  True],
       fill_value=999999)

In [7]: np.clip(m, 0, 5, m)                                                                                                                                                
Out[7]: 
masked_array(data=[0, 1, 2, 3, 4, 5, 5, 5, 5, 5],
             mask=False,
       fill_value=999999)

In [8]: m                                                                                                                                                                  
Out[8]: 
masked_array(data=[0, 1, 2, 3, 4, 5, 5, 5, 5, 5],
             mask=False,
       fill_value=999999)

I also opened an issue in numpy's repo: numpy/numpy#14140

@jklymak
Copy link
Member

jklymak commented Jul 27, 2019

Looks good. Were we ignoring an API change warning on this?

@jklymak
Copy link
Member

jklymak commented Jul 27, 2019

Is there a reason codecov didn’t trigger for this PR?

@bingyao
Copy link
Contributor Author

bingyao commented Jul 27, 2019

@jklymak Thanks for the reviewing! But I don't quite follow your two comments here: API change warning? and codecov didn't trigger? 😝What are they means?

@jklymak
Copy link
Member

jklymak commented Jul 27, 2019

We have a codecoverage check called codecov that makes sure your change is actually tested.

Numpy obviously changed their API. If we weren’t getting deprecation notices they may have changed this inadvertently and it should be reported to them.

@tacaswell tacaswell added this to the v2.2.5 milestone Jul 28, 2019
@tacaswell
Copy link
Member

I'm going to merge this because it un-breaks the build.

Thanks @bingyao for the fix and for reporting it upstream!

@tacaswell tacaswell merged commit 667a100 into matplotlib:master Jul 28, 2019
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Jul 28, 2019
@tacaswell
Backport PR matplotlib#14901: Fix GH14900: numpy 1.17.0 breaks test_c…
d04f430 
…olors.
@meeseeksmachine meeseeksmachine mentioned this pull request Jul 28, 2019
Merged
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Jul 28, 2019
@tacaswell
Backport PR matplotlib#14901: Fix GH14900: numpy 1.17.0 breaks test_c…
8b398bf 
…olors.
@meeseeksmachine meeseeksmachine mentioned this pull request Jul 28, 2019
Merged
tacaswell added a commit that referenced this pull request Jul 28, 2019
@tacaswell
Merge pull request #14910 from meeseeksmachine/auto-backport-of-pr-14…
1230e83 
…901-on-v3.1.x

Backport PR #14901 on branch v3.1.x (Fix GH14900: numpy 1.17.0 breaks test_colors.)
@eric-wieser
Copy link
Contributor

The change in numpy was indeed an accident, and will hopefully restore the correct behavior in 1.17.1. I don't know if you want to do a version-check here for just the broken release, and keep the more memory-efficient out argument otherwise?

@bingyao bingyao deleted the fix_tests_with_numpy_1_17 branch July 29, 2019 02:07
QuLogic added a commit that referenced this pull request Aug 2, 2019
@QuLogic
Merge pull request #14909 from meeseeksmachine/auto-backport-of-pr-14…
20fa909 
…901-on-v2.2.x

Backport PR #14901 on branch v2.2.x (Fix GH14900: numpy 1.17.0 breaks test_colors.)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jklymak jklymak jklymak approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v2.2.5
Development

Successfully merging this pull request may close these issues.

Numpy 1.17.0 breaks test_colors
4 participants
@bingyao @jklymak @tacaswell @eric-wieser