Skip to content

sanitize norm extrema to be floats #10721

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

Merged
merged 1 commit into from
Mar 9, 2018

Conversation

ngoldbaum
Copy link
Contributor

PR Summary

Fix regression from 2.1.2 by ensuring norm extrema are always floats.

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant

PR Description

We had a report from a yt user complaining about saving plots to disk raising errors in some cases:

https://mail.python.org/mm3/archives/list/yt-users@python.org/thread/24OJRJVPK47RMG7XLF7J7YP4JQH24MG7/

I was able to reproduce this issue on matplotlib 2.2.0 but not on matplotlib 2.1.2 and earlier, so this is a regression.

I believe this behavior was unintentionally introduced by @anntzer in #6700 to avoid rounding issues with float128 on linux. As a side-effect that PR changed how matplotlib deals with data with units attached.

I've added a _sanitize_extrema helper so that we can use np.asscalar to avoid downcasting float128 data while still converting data to a numpy scalar under the hood. I've also expanded the existing test in test_colors.py that handles ndarray subclass to also check this case as well.

It would be great if this could be backported to 2.2.

@jklymak
Copy link
Member

jklymak commented Mar 8, 2018

What type of data was failing? Does the test cover the failure mode?

This seems reasonable to me, but it'd be good to make sure we know what failed and that we have a test to gaurd for future..

@jklymak jklymak added this to the v2.2.1 milestone Mar 8, 2018
@jklymak jklymak added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Mar 8, 2018
@ngoldbaum
Copy link
Contributor Author

What type of data was failing? Does the test cover the failure mode?

If you set vmin or vmax in an imshow call to be a quantity with units that caused failures later. This wasn't an issue before 2.2.0.

This seems reasonable to me, but it'd be good to make sure we know what failed and that we have a test to gaurd for future..

The changes I made to test_colors.py should test this failure mode.

@jklymak
Copy link
Member

jklymak commented Mar 8, 2018

If I run the following on master and v2.2.0, it comes back True, so I don't think this test is triggering the bug, or this problem is fixed on master.

import numpy as np
import matplotlib
import matplotlib.colors as mcolors

class MyArray(np.ndarray):
    def __isub__(self, other):
        raise RuntimeError

    def __add__(self, other):
        raise RuntimeError

data = np.arange(-10, 10, 1, dtype=float)
mydata = data.view(MyArray)
for norm in [mcolors.Normalize(vmin=mydata.min(), vmax=mydata.max()),
            mcolors.SymLogNorm(3, vmin=mydata.min(), vmax=mydata.max())]:
    print(np.all(norm(mydata) == norm(data)))

@ngoldbaum
Copy link
Contributor Author

@jklymak thanks! It looks like you actually have to use the Normalize instance with an imshow plot to trigger the bug. Let me see if I can rewrite the test in a way that actually triggers it. I appreciate your patience with my test writing :)

@jklymak
Copy link
Member

jklymak commented Mar 8, 2018

Ha, given that I just got a slap on the wrist for my test writing, please no apologies!

We did change some image normalization stuff recently, hence I wanted to make sure that we didn't miss something. i.e. #10613 worse, that one didn't get in for the RC, but was in response to a bug report after the RC, so its possible we (ahem, I) fixed one thing and broke another.

@ngoldbaum
Copy link
Contributor Author

@jklymak updated the test. Here's a script that I verified should fail on 2.2.0:

import matplotlib
matplotlib.use('Agg')
import numpy as np
import matplotlib.colors as mcolors
from matplotlib import pyplot as plt


class MyArray(np.ndarray):
    def __isub__(self, other):
        raise RuntimeError

    def __add__(self, other):
        raise RuntimeError


data = np.arange(-10, 10, 1, dtype=float)
data.shape = (10, 2)
mydata = data.view(MyArray)
for norm in [mcolors.Normalize(vmin=mydata.min(), vmax=mydata.max()),
             mcolors.SymLogNorm(3, vmin=mydata.min(), vmax=mydata.max())]:
    fig, ax = plt.subplots()
    ax.imshow(mydata, norm=norm)
    fig.canvas.draw()

@jklymak
Copy link
Member

jklymak commented Mar 8, 2018

Yep, that was #10613. Sorry about that. Your fix looks reasonable, as does some test along the lines of the above.

Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @ngoldbaum !

@dopplershift dopplershift merged commit c85b029 into matplotlib:master Mar 9, 2018
QuLogic added a commit that referenced this pull request Mar 10, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants