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

Range bug fix for pcolor and pcolormesh #1314

Merged
merged 4 commits into from Oct 19, 2012
Merged

Conversation

bblay
Copy link
Contributor

@bblay bblay commented Sep 26, 2012

Now transforming, if necessary, before calculating data range.

@pelson
Copy link
Member

pelson commented Sep 26, 2012

@bblay you will definitely need tests for this. See #1265 for the same fix applied to contour.

I will happily advise on this PR, but due to conflicts of interest, I will defer any merging to @mdboom or other mpl developer.

Thanks,

@@ -7352,6 +7352,20 @@ def pcolor(self, *args, **kwargs):

x = X.compressed()
y = Y.compressed()

# Transform from native to data coordinates?
t = collection._transform
Copy link
Member

Choose a reason for hiding this comment

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

Can this be None? In some other artists it can, but perhaps not here...

Copy link
Member

Choose a reason for hiding this comment

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

I think the None case is handled by the next line, no?

Copy link
Member

Choose a reason for hiding this comment

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

Agreed. The None case is handled in exactly the same way as it was previously.

@WeatherGod
Copy link
Member

Here is another one that probably should get into rc3, but I don't see any tests.

@bblay
Copy link
Contributor Author

bblay commented Oct 15, 2012

Tests. Apologies for the delay.

Also:
Thought I should make a test class to encapsulate commonality but was a little uncertain about the @cleanup.

@dmcdougall
Copy link
Member

My feeling is that this should be an image comparison test. What are everyone else's thoughts?

@mdboom
Copy link
Member

mdboom commented Oct 15, 2012

I think in this particular case it's fine without the image comparison -- testing the view limits should be enough. But I also could be missing an important failure mode that an image comparison test would find.

@dmcdougall
Copy link
Member

The test_pre_transform_plotting failure is because it uses pcolormesh in the test. Since @bblay modified the pcolormesh method, the baseline image for that test needs to be recreated.

@dmcdougall
Copy link
Member

@pelson I'm not a transforms expert. Are you happy with the fix for this, since you wrote one for contour?

if t and any(t.contains_branch_seperately(self.transData)):
trans_to_data = t - self.transData
pts = np.vstack([X, Y]).T.astype(np.float)
transformed_pts = trans_to_data.transform(pts)
Copy link
Member

Choose a reason for hiding this comment

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

The only problem with this (and the fix that I implemented), is that the result of transform could be NaN/Inf. It could be fixed in this PR, or we could do it for 1.3 - I will leave that call to whomsoever merges this PR.

Copy link
Member

Choose a reason for hiding this comment

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

@pelson Under what circumstances would this case occur?

Copy link
Member

Choose a reason for hiding this comment

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

Try uncommenting the set_?lim calls and the data is not put in the correct range.

import matplotlib.pyplot as plt
import numpy as np


import matplotlib.transforms as mtrans


class div(mtrans.Transform):
    input_dims = 2
    output_dims = 2
    has_inverse = False

    def transform_non_affine(self, points):
        new_points = points.copy()
        new_points[:, 1] = 1/new_points[:, 1]
        return new_points

ax = plt.axes()
npts = 21
xs = np.linspace(-5, 10, npts)
ys = np.linspace(0, 5, npts)
xs, ys = np.meshgrid(xs, ys)
data = xs**2 + ys**2

plt.contourf(xs, ys, data, transform=div() + ax.transData)

ax.set_xlim([-6, 11])
ax.set_ylim([0, 5])

plt.show()

Changing the transform to fix inf points will solve the issue:

    def transform_non_affine(self, points):
        new_points = points.copy()
        new_points[:, 1] = 1/new_points[:, 1]
        new_points[new_points == np.inf] = 10
        return new_points

This means, if inf/nan is ignored at the level that this PR fixes, then we will get the "correct" limits.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, by range you mean the x/y range, not the "z" range.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, even with @bblay's fix, the data is still not put in the correct range...

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind; I was being retarded.

@pelson
Copy link
Member

pelson commented Oct 16, 2012

@pelson I'm not a transforms expert. Are you happy with the fix for this, since you wrote one for contour?

Looks fine to me. I have commented that there is a limitation of this approach, but it is fairly limited (although I have hit it), and only applies when pre-transforming of data is taking place.

@pelson
Copy link
Member

pelson commented Oct 16, 2012

👍

@dmcdougall
Copy link
Member

Ok, I finally understand what bug this fixes. I give this my +1 now. The assert test is fine with me, too.

Regarding inf and nan, that can be done later. And would probably only come about if you were using transforms with singularities.

@dmcdougall
Copy link
Member

@bblay There is a rc3 tag scheduled for this Friday. Though the schedule is often not rigid, I think merging it by close of play tomorrow (Thursday) would be a good idea. Before that can happen, the baseline image for the failing test needs to be regenerated. If you have time would you be able to do that? If not, let me know and I will fork your branch and add it. Thanks again for the hard work.

@bblay
Copy link
Contributor Author

bblay commented Oct 17, 2012

Two baseline images were updated, one of which is an svg.
Can't see the svg changes in github but I can't see any difference when flicking between the old and new versions.

@dmcdougall
Copy link
Member

@bblay If you click the "Files Changed" tab at the top of this page you can scroll down to the png file. There are some options just under the pngs. If you click 'diff' you'll see an image diff between the two baseline images. Furthermore 'Onion skin' lets you slide between the two. At the very least there are some font alignment issues. We've had problem with test images and fonts in the past, so I don't know what's going on there. There's also a one-pixel line change in one of the parts of the image.

I don't know about you but the updated image looks 'better', in some sense.

I'll merge this by the end of tomorrow, so people have a chance to chime in regarding the updated baseline image if they have expertise they wish to add.

Thanks for seeing to this.

Edit: grammar.

@ghost ghost assigned dmcdougall Oct 17, 2012
@dmcdougall
Copy link
Member

Merging. Thanks for the hard work @bblay.

dmcdougall added a commit that referenced this pull request Oct 19, 2012
Range bug fix for pcolor and pcolormesh
@dmcdougall dmcdougall merged commit a3ec8b0 into matplotlib:v1.2.x Oct 19, 2012
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.

None yet

5 participants