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

Adding a get_coordinates() method to Quadmesh collections #18472

Merged
merged 1 commit into from May 26, 2021

Conversation

greglucas
Copy link
Contributor

PR Summary

This returns the coordinates used to create the Quadmesh collection. These can be useful for knowing what vertices were used to generate the mesh returned from pcolormesh.

closes #18399

PR Checklist

  • Has pytest style unit tests (and pytest passes).
  • Is Flake 8 compliant (run flake8 on changed files to check).
  • New features are documented, with examples if plot related.
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • Conforms to Matplotlib style conventions (install flake8-docstrings and pydocstyle<4 and run flake8 --docstring-convention=all).
  • New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
  • API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

jklymak
jklymak previously approved these changes Apr 23, 2021
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.

Seems simple enough

@QuLogic
Copy link
Member

QuLogic commented May 15, 2021

This is probably okay, but I'm unsure about the name. For pcolormesh, I'm not sure I consider the x/y coordinates the "data". I'm not sure we have anything else that works quite similarly, though, @timhoffm?

@QuLogic QuLogic added the status: needs comment/discussion needs consensus on next step label May 15, 2021
@greglucas
Copy link
Contributor Author

See the linked issue for other ideas too. My initial idea was to call it get_coordinates(), but then saw Lines had get_xydata(), so then changed to that for consistency. I agree though it is a bit confusing and I think my preference would be get_coordinates(). I'll wait until others speak up with a preference one way or the other to change anything.

@jklymak
Copy link
Member

jklymak commented May 15, 2021

Yeah, I didn't think about the name. get_xydata makes sense for Line2D but thats because x,y are the data whereas here x and y are the coordinates. These are called coordinates internally, so I'd suggest they are called this publicly as well. I guess the question is if we want to also allow set_coordinates?

@jklymak jklymak dismissed their stale review May 15, 2021 15:24

Maybe need to change name...

@greglucas
Copy link
Contributor Author

I really didn't want to go down the rabbit hole of adding set_coordinates() as well, and I think that may be even more confusing because the Quadmesh init() takes flattened coordinates as an argument:

def __init__(self, meshWidth, meshHeight, coordinates,
antialiased=True, shading='flat', **kwargs):
super().__init__(**kwargs)
self._meshWidth = meshWidth
self._meshHeight = meshHeight
# By converting to floats now, we can avoid that on every draw.
self._coordinates = np.asarray(coordinates, float).reshape(
(meshHeight + 1, meshWidth + 1, 2))

which are promptly reshaped to (ny, nx, 2) to store internally, so which one of those versions do we support?

Digression...

Honestly, my real preference would be to deprecate the flattened version of Quadmesh altogether and only take the actual 3D mesh as input (or you could do xcoord, ycoord as well to make it more like pcolormesh). There really isn't a need to pass meshWidth and meshHeight into the constructor, you could easily just get that from the shape of the coordinates that were passed in. Now that I'm writing this out, I'm wondering if there would be buy-in from others on the idea of deprecating the flattened QuadMesh? I don't think too many people are creating QuadMesh's from the constructor, but rather it is usually a returned Class from pcolormesh and other higher-level plotting functions. So, a deprecation may not hit too many people actually and would hopefully benefit them by not forcing them to reshape/flatten.

@timhoffm
Copy link
Member

timhoffm commented May 15, 2021

I agree that flattened QuadMesh coordinates are awkward. It's the relation of 4 points that makes up a quadrilateral. Within the flattened data, we still imply a certain ordering in rows/columns. If we/the user gets that wrong, we'll end up with a total mess. Example (changing the order of points by reshape (4, 3) -> (3, 4)):

Show code
x = [0, 1, 2]
y = [2, 4, 6, 8]
z = np.arange(6).reshape(3, 2)
xx, yy = np.meshgrid(x, y)
fig, (ax1, ax2) = plt.subplots(1, 2)
ax1.pcolormesh(xx, yy, z, edgecolor='r')
ax2.pcolormesh(xx.reshape(3, 4), yy.reshape(3, 4), z.reshape(2, 3), edgecolor='r')

grafik

So, it seems better to make the relation explicit by a 2D shape.

There are multiple parts to this:

  1. The new get_coordinates() (naming t.b.d.) here should directly return (ny, nx, 2).
  2. QuadMesh.__init__ transition will take multiple steps. I'll can make a separate PR on this including the transition path from
    __init__(self, meshWidth, meshHeight, coordinates, antialiased=True, shading='flat', **kwargs)
    to
    __init__(self, coordinates, *, antialiased=True, shading='flat', **kwargs)
  3. set_array/ get_array() inherited from ScalarMappable
  4. Multiple elements from Collection operate on the flattened data as well, e.g. edgecolors (get/set/kwarg) etc.

It will in particular be hard if not impossible to change everything in the fourth category. The 1D logic of ScalarMappable and Collection will always shine through in some places. However, that probably should not hold us up from changing 1., 2., and maybe 3. The alternative is having a consistently unusable API everywhere.

Return the x and y vertices of the mesh.

The shape of the returned coordinates is
(*meshWidth* + 1, *meshHeight* + 1, 2).
Copy link
Member

Choose a reason for hiding this comment

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

Does the user know what meshWidth/Height are?

Maybe a bit more explanation?

Suggested change
(*meshWidth* + 1, *meshHeight* + 1, 2).
(*meshWidth* + 1, *meshHeight* + 1, 2), where *meshWidth* and *meshHeight* are the size of the
mesh, passed in when the Artist was initialized.

Copy link
Contributor Author

@greglucas greglucas May 16, 2021

Choose a reason for hiding this comment

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

Hmm... Good point. Unfortunately, the initialized wording isn't entirely clear to me either because pcolormesh can auto-expand the coordinates for you (which is why I wanted this public in the first place). The docstring for QuadMesh gives some info on what meshWidth/meshHeight are, so maybe that is sufficient for now?
https://matplotlib.org/stable/api/collections_api.html#matplotlib.collections.QuadMesh

Comment on lines 2024 to 2027
Return the x and y vertices of the mesh.

The shape of the returned coordinates is
(*meshWidth* + 1, *meshHeight* + 1, 2).
Copy link
Member

@timhoffm timhoffm May 25, 2021

Choose a reason for hiding this comment

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

We're removing the names meshWidth/meshHeight from code and docs.

Some thing like this should do:

Suggested change
Return the x and y vertices of the mesh.
The shape of the returned coordinates is
(*meshWidth* + 1, *meshHeight* + 1, 2).
Return the vertex positions of the mesh as (M, N, 2) array.
M, N are the number of rows / columns in the mesh.
The last dimension specifies the components (x, y).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good. The only issue is that it may be confusing to use (M, N) here to refer to the vertices when other places use that notation to refer to the number of Polygons/Quads.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, technically this might be (M+1, N+1, 2) array. Would that be better?

More generally, I'm switch back and forward between the +/-1 interpretations, i.e. what is the canonical quantity for the number (M, N), the quadrilaterals or the vertices/coordinates?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Most of the previous documentation has had (M, N) as the number of quadrilaterals in the mesh, and to me that makes intuitive sense since that is the quantity we are most interested in. I'd vote for changing this to (M+1, N+1, 2) and explaining that here, but I also don't have a real strong preference.

Copy link
Member

Choose a reason for hiding this comment

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

I do - the data is MxN, the co-ordinates follow that. You could argue the coordinates are more important if they are twisting out some complicated shape, but 99.9% of the time this is tracking out a regular grid.

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 changed the text to explicitly call out and distinguish the quads and vertices in the statement.

Return the vertices of the mesh as an (M+1, N+1, 2) array.

M, N are the number of quadrilaterals in the rows / columns of the mesh, corresponding to (M+1, N+1) vertices. The last dimension specifies the components (x, y).

@greglucas greglucas force-pushed the quadmesh_get_xydata branch 2 times, most recently from 86c6e75 to f52bbc1 Compare May 26, 2021 02:23
@greglucas greglucas changed the title Adding a get_xydata() method to Quadmesh collections Adding a get_coordinates() method to Quadmesh collections May 26, 2021
This returns the coordinates used to create the Quadmesh collection.
@tacaswell tacaswell merged commit 9091ba9 into matplotlib:master May 26, 2021
@greglucas greglucas deleted the quadmesh_get_xydata branch May 26, 2021 17:25
@timhoffm
Copy link
Member

Thanks @greglucas for keeping working on this and going through all the review cycles with us!

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.

How to get Quadmesh coordinates
5 participants