Skip to content

Fix inconsistent behavior in pcolormesh with Gouraud shading #9594

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

Closed
wants to merge 7 commits into from

Conversation

simonpf
Copy link

@simonpf simonpf commented Oct 27, 2017

PR Summary

As described in issue 8422 behavior of pcolormesh was inconsistent when using Gouraud shading. The documentation states that the X and Y grids, if given, should have the same or one row and column more than the color grid C. However, if Gouraud shading is used, this would result in an error.

To handle the case where the X and Y grid have on row and column more than the color grid, I propose to interpolate the grids linearly to obtain the center points of each quadrilateral. This should ensure that the resulting image has the right colors at the centers of each of the quadrilaterals given.

The disadvantage, however, is that the plotted grid will cover a smaller range than X and Y grids given to pcolormesh. It is up for discussion if the grid should be so avoided by padding the resulting grids.

Example

import numpy as np
import matplotlib.pyplot as plt
x = np.arange(-1, 2)
y = np.arange(-1, 2)
xe = np.linspace(-1.5, 1.5, 4)
ye = np.linspace(-1.5, 1.5, 4)
c = np.exp(-0.5*0.125*(x.reshape(1, -1)**2 + y.reshape(-1, 1)**2))
plt.figure()
plt.pcolormesh(xe, ye, c, shading='gouraud')  # didn't work before

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@efiring
Copy link
Member

efiring commented Oct 27, 2017

I don't understand; what is wrong with just setting 'allmatch' to True if Gouraud shading is used, and letting it error out when the dimensions don't match? Then it is up to the user to decide how to handle the situation. A little more documentation, and maybe more info in the error message, seems to me to be all that is needed. Otherwise we are trying to ignore the fundamental difference between Gouraud shading and normal pcolormesh.

But if this suggestion is rejected, then my fallback would be to chop the offending extra values off the edges of the X and Y input arrays. This would be consistent with the present Matlab-compatibility quick-and-dirty practice of chopping edges off the Z array with flat shading.

@anntzer
Copy link
Contributor

anntzer commented Oct 27, 2017

I think we should instead just document properly what are the required dimensions (with flat shading, X and Y are boundaries; with gouraud shading, they match "centers") and error out cleanly in other cases (with a deprecation for the case of flat shading):

Errors should never pass silently.
Unless explicitly silenced.
In the face of ambiguity, refuse the temptation to guess.

Note that right now, with mismatched dimensions, you can get, e.g.

Dimensions of C (3, 3) are incompatible with X (4) and/or Y (4); see help(pcolormesh)

but the docstring of pcolormesh says nothing about X and Y's dimensions.

@simonpf
Copy link
Author

simonpf commented Oct 27, 2017

I basically followed the suggestions in the corresponding issue.

I totally agree that if the user provides inconsistent data an error should be thrown. But I guess the argumentation is that the for flat shading the case when inconsistent data is provided IS handled while for Gauroud Gouraud it is not.

@jklymak
Copy link
Member

jklymak commented Oct 27, 2017

Yes please don’t drop support for same-dimensions for pcolormesh! I think the case for adding support for n+1 gouraud is strong as well. Often when pcoloring you just want to switch between methods, and having to change x and y is a pain.

@anntzer
Copy link
Contributor

anntzer commented Oct 27, 2017

@jklymak I have the feeling that what you are asking for is really proper interpolation support for pcolor, no (not saying that it'd be easy to do...)?

@jklymak
Copy link
Member

jklymak commented Oct 27, 2017

I don't think so. I'm suggesting that if I input x = [0 2 4 8] but only have three columns, pcolormesh intelligently treats this as x = [1 3 6] for shading that requires this...

@anntzer
Copy link
Contributor

anntzer commented Oct 27, 2017

TBH it's exactly the kind of behavior I want to stamp out.

More precisely, the behavior proposed in this PR (ie what you propose) for pcolormesh only slightly annoys me, because I think it is doing too much in the back of the user; I would prefer just providing a helper function that does the regridding.

On the other hand, I really believe we should never silently drop (even a small part of) user input, as is currently the case for pcolor with equal-sized inputs.

@jklymak
Copy link
Member

jklymak commented Oct 27, 2017

I'd say 85% of the plotting I do is with matrices where z is MxN, x is N and y is M. Semantically, in my mind, x = [1 3 6] and z has three columns mean I want the center of the facet defined by z. I appreciate the other semantic is valid - that x = [0 2 4 8] means the edge of the facets and some sort of interpolation happens between them - but I don't see one set as superior to the other.

I don't think Matplotlib should make users be aware of implimentation details unless they need to be made aware of them. Of course if someone zooms in on the individual pixels and gets upset about the alignment, then they need to figure out the implimentation details and they should be clear in the docs (which I think they are). But 98% of users just want the plot to work w/o a bunch of futzing...

@efiring
Copy link
Member

efiring commented Oct 27, 2017

I'm more with @anntzer on this: move in the direction of clarity, explicitness, and correctness, with helper functions as needed for ease of use. Keyword arguments could also be used to explicitly request certain behaviors. I think that with careful design we can have both correctness and ease of use. And less confusion.

@jklymak
Copy link
Member

jklymak commented Oct 28, 2017

Ok but just to be clear about this 99.9% of netcdf files (for instance) have variables that have the same dimensions as the accompanying coordinate variables. It’s the usual way to layout data. I don’t think the default for pcolormesh should be that the user has to mess with a perfectly normal data layout and figure out where the coordinate bin edges are located.

@simonpf
Copy link
Author

simonpf commented Oct 29, 2017

OK, so I tried to combine the points brought up in the discussion and came up with the following changes:

  • Clearly documented requirements on X and Y in docstring of pcolormesh
  • In the case of Gouraud shading, throw an error if X and Y do not match C in dimension
  • Added interpolate_grids keyword, which when set to True will avoid failure in the above case
    by interpolating the grids X and Y.

@tacaswell tacaswell added this to the v2.2 milestone Oct 29, 2017
@tacaswell
Copy link
Member

This will need an entry in api_changes as it will break various size data working and a whats_new for the new kwarg.

Would a reasonable middle ground to have the kwarg default to None which means 'guess if you have to, but if you do warn the user to pass True as this might become strict in the future'?

@anntzer
Copy link
Contributor

anntzer commented Oct 29, 2017

This discussion reminds me of #5855 where I was on the other end of the argument (and that was a case where AFAICT there was really only one way to fix the data, and that involved no data point being ignored...).

@jklymak
Copy link
Member

jklymak commented Oct 30, 2017

In my opinion:

  • doing the grid interpolation for shading='gouraud' is great.
  • having two funny flags for this case (allmatch, and interpolate_grids) is not great. I'd just do the interpolation, and log under verbose.helpful or logging.info that this happened. I can't imagine a case where a user is going to complain that they got a plot instead of an error telling them to futz with their dimensions.

Further, I think the solution for shading='flat' and z = MxN, x = N, and y =M, is pretty suboptimal. If it were me, I'd have increased the gridsize sent to the shading by one rather than decrease the data size by one. I'm not clear why the grid would get primacy over the data...

@simonpf
Copy link
Author

simonpf commented Nov 1, 2017

Just to clarify:

The new interpolate_grids keyword is False by default, so an error will be thrown just as before when X and Y do not match C.

To enable the interpolation the user has to actively set interpolate_grids to True. As far as I can see, this should not break any old code or am I missing something?

@jklymak
Copy link
Member

jklymak commented Nov 1, 2017

I'm just arguing that you shouldn't have the flag, and just do the interpolation no matter what.

If you do go the way of having the flag, then the error message should at least mention it.

@simonpf
Copy link
Author

simonpf commented Nov 2, 2017

@jklymak, I decided to have the flag because from the previous discussion it seems that the majority is against having the interpolation by default. But as you suggested I added the mention of the interpolate_grids keyword argument.

@tacaswell Is the api_changes entry still required even if this will not break the API?

@jklymak jklymak marked this pull request as draft September 12, 2020 19:56
@jklymak
Copy link
Member

jklymak commented Sep 12, 2020

This will need revision based on new pcolormesh behaviour. Not sure if @simonpf is still interested in pursuing this.

@tacaswell
Copy link
Member

In the intervening time we have made some changes to this signature, it is now https://matplotlib.org/stable/api/_as_gen/matplotlib.axes.Axes.pcolormesh.html#matplotlib.axes.Axes.pcolormesh and addded a shading 'auto' that picks the correct version of 'flat' and 'nearest', but each of the shading modes is now strict about the expected shape of the input. I do not think we should re-open the discussion of automatically converting from the expected shapes.

Given that these changes over took this PR I am going to close it. I apologize that this got stalled in review @simonpf and thank you for you work! I hope we will hear from you again.

@tacaswell tacaswell closed this Dec 5, 2022
@QuLogic QuLogic removed this from the future releases milestone Dec 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants