8000 Make pcolor more mesh-like by greglucas · Pull Request #25027 · matplotlib/matplotlib · GitHub
[go: up one dir, main page]

Skip to content

Make pcolor more mesh-like #25027

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
Jul 6, 2023
Merged

Conversation

greglucas
Copy link
Contributor
@greglucas greglucas commented Jan 19, 2023

PR Summary

This is a follow-on from #24638
and makes pcolor more similar to pcolormesh with 2D arrays of x/y/c and just uses a different draw implementation. This should maintain backwards compatibility as we are subclassing the PolyCollection still to use that draw method.

  • pcolor: draw every quad individually as Polygons
  • pcolormesh: draw all quads together (depending on backend implementation)

Basically, this is an attempt to allow all inputs/getters/setters to be able to use 2D meshes rather than requiring raveling arrays and needing to remember when to use 1D/2D quantities for various functions.

closes #25770

PR Checklist

Documentation and Tests

  • Has pytest style unit tests (and pytest passes)
  • [N/A] Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • [N/A] New plotting related features are documented with examples.

Release Notes

  • [N/A] New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
  • [N/A] API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
  • [N/A] Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

Copy link
Member
@efiring efiring left a comment

Choose a reason for hiding this comment

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

I'm strongly in favor of this, but I have questions about one detail and one matter of strategy.

X = np.hstack((X[:, [0]] - dX[:, [0]],
X[:, :-1] + dX,
X[:, [-1]] + dX[:, [-1]]))
if isinstance(X, np.ma.core.MaskedArray):
Copy link
Member

Choose a reason for hiding this comment

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

To be more compact you can use if np.ma.isMA(X): or if np.ma.isMaskedArray(X):. Both just call isinstance(...). If you want to use the explicit isinstance, you can still make it more compact be deleting the .core part. The most compact version would be

hstack = np.ma.hstack if np.ma.isMA(X) else np.hstack

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 I also cleaned up the handling above this as well.

@@ -2111,3 +2111,39 @@ def get_cursor_data(self, event):
if contained and self.get_array() is not None:
return self.get_array().ravel()[info["ind"]]
return None


class PolyQuadMesh(PolyCollection, QuadMesh):
Copy link
Member

Choose a reason for hiding this comment

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

Would it make more sense to reverse the inheritance order, and use "super" below? Is the PolyQuadMesh more like a QuadMesh than a PolyCollection, falling back on the latter only for its draw method, as is already done explicitly below? Or is the current order correct with respect to all inherited methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we actually want most of the methods from PolyCollection because we are drawing Polygons and want that set_paths() method. We really want the QuadMesh coordinates stored internally is all I think. I reworked the inheritance a bit in the most recent commit.

One thing I need to think about a bit more is how to handle the masking of paths here... len(self._paths()) != coords[:2].size when there are masked elements. So, we may actually have to overwrite the set_paths() here to make sure that we are updating the proper facecolors to correspond to the mesh.

Currently, we return a PolyCollection that has a smaller number of paths, so if you want to update the array with the proper colors you need to account for that resizing yourself. (We do that in Cartopy, so perhaps just porting that upstream here is the way to go)

Copy link
Member

Choose a reason for hiding this comment

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

It turns out my understanding of multiple inheritance was, and probably still is, incomplete. There are arguments in favor of "composition instead of inheritance", and I wonder whether this is an example where using composition--making an internal QuadMesh instance, and referring to its contents explicitly when needed--might be more readable, robust, and maintainable. It would reduce the inheritance to single--much easier to fit in one's brain.

Copy link
Member

Choose a reason for hiding this comment

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

Or, is there a part of QuadMesh code that should be factored out into a mixin?

8000

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a _MeshData mixin as a separate commit. Let me know your thoughts on how that organization seems now.

Copy link
Member

Choose a reason for hiding this comment

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

The mixin looks fine to me.

@greglucas
Copy link
Contributor Author

Discussed this on the call today and there was some interest in pursuing the class mixin idea here to try and make the multiple-inheritance a little bit more manageable.

We want the coordinate handling of QuadMesh, but the drawing capability of PolyCollection. The mixin will likely look like some 2D coordinate handling class without a .draw() method. Then the current QuadMesh would still implement its own .draw() relying on the coordinate handling from the mixin. This would eliminate some of the confusion for where the coordinate handling/drawing are coming from if the mixin is designed to only handle the 2D coordinate work and not the drawing.

class QuadCoordinatesMixin
    def __init__(self, coordinates):
        self._coordinates = coordinates
    
    def set_array(self, A):
        # current quadmesh array validation logic for the shapes

    def get_coordinates(self):
        return self._coordinates

    # ... other (static)methods like get/set_paths()?

 class QuadMesh(QuadCoordinatesMixin, Collection):
    # override draw/path methods from Collection

class PolyQuadMesh(QuadCoordinatesMixin, PolyCollection):
    # override draw/path methods from PolyCollection to utilize 2D coordinates from the mixin

Other thoughts and comments on the design are more than welcome!

@jklymak jklymak added the status: needs comment/discussion needs consensus on next step label Feb 6, 2023
@jklymak
Copy link
Member
jklymak commented Feb 20, 2023

This seems to be in draft state, but feel free to move back...

@greglucas
Copy link
Contributor Author

@rcomer, I think it was fairly straight forward to support RGB(A) here as well. I just pushed a new commit that seems to work well with that case, although I didn't add any tests yet.

@efiring, pinging you to get your thoughts on this restructure again.

@rcomer
Copy link
Member
rcomer commented Apr 29, 2023

I tried this out with Cartopy, and it breaks the usage there because Cartopy expects a masked array to be compressed and subsequently passes an array to set_array based on that assumption. Here is a simple example:

import matplotlib.pyplot as plt
import numpy as np

arr = np.ma.array(np.arange(12).reshape(3, 4), mask=[[0, 0, 0, 0],
                                                     [1, 1, 1, 1],
                                                     [0, 1, 0, 1]])
               
fig, ax = plt.subplots()
pc = ax.pcolor(arr)

pc.set_array(np.ones(6))

plt.show()

With v3.7 and main, this gives

test

With this branch we get

ValueError: For X (5) and Y (4) with flat shading, A should have shape (3, 4, 3) or (3, 4, 4) or (3, 4) or (12,), not (6,)

There is also handling in Cartopy that expects to get the compressed array out of get_array. It's very easy to fix all of this in Cartopy, but I wonder if there should be a deprecation pathway?