-
Notifications
You must be signed in to change notification settings - Fork 44
Edge plotting method that extracts visible edges automatically #1388
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
base: main
Are you sure you want to change the base?
Conversation
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
@nils3er thanks for this PR. Some thoughts: We already support plotting edges via uxgrid.plot.edges() through our plotting accessor. Your new method uxgrid.plot_edges() creates a parallel API that does similar functionality but bypasses our established plotting patterns. Questions:
|
Hi @rajeeja here is the whole story: In yac we moved our weight visualization tool to uxarray for the internal grid representation. For that we need to plot the edges of a (maybe high-res) grid on a local cartopy axes. The straight-forward way, which is also described in the documentation is: ax = plt.axes(projection=ccrs.PlateCarree())
ax.set_extent([-60, -40, -10, 10])
lc = g.to_linecollection()
ax.add_collection(lc) That works, but for example for a healpix zoom 5 grid, this already takes 10s on my system. Unfortunately I don't managed to get the However, when thinking about this, a more uxarray-ish solution came to my mind: ax = plt.axes(projection=ccrs.PlateCarree())
ax.set_extent([-60, -40, -10, 10])
g_local = g.subset.bounding_circle((-50,0), 20)
lc = g_local.to_linecollection()
ax.add_collection(lc) This also speeds up the plotting and finding the relevant edges is left to the xarray implementation which could then be optimized for the grid. The downside is that it is probably difficult to find a good bounding_circle where the whole extent is included in. I'm open for discussions here and of course the API needs to be streamlined with the existing one. |
You are right, hvplot's "create its own figure" and that approach doesn't fit the YAC workflow you describe. Thanks for the detailed info. I have verified and indeed the speedup is significant. How about we enhance the main grid.plot.edges() interface to automatically detect matplotlib+cartopy contexts and route to your optimized method behind the scenes? Maybe this is also a chance for us to look at a broader plotting ways in other places where we default to hvplot.@erogluorhan |
@nils3er , thanks very much for this contribution; I had seen it in your notebook that you shared with us a few days ago and already liked the idea, and now it is great to see it coming UXarray's way. Regarding how to integrate it to UXarray's plotting API design though, I think we will need to re-think about the points @rajeeja made (maybe will end up wanting to change, flex some parts of the existing design). I thought about this slightly when I was looking into your notebook but wasn't able to come up with a solution that I was convinced yet. Basically, we have the Now that your contribution offers an edge-plotting technique that would be very useful for UXarray's grids, and given the not-so-great perfoormance of our edge plotting that uses @rajeeja and I can spend some time thinking and maybe pair-programming on this and get back to you with some ideas. Does it make sense? |
def plot_edges( | ||
self, | ||
ax: Optional["cartopy.mpl.geoaxes.GeoAxes"] | None, # noqa: F821 | ||
**plot_kwargs, | ||
): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned by @erogluorhan, if we could design this function similar to how we set up to_raster()
, that would be preferred to avoid conflicting names with our .plot
accessors.
to_raster
also doesn't directly call any ax.plot
routines. To be consistent with that, having this method return something that can then be externally passed into ax.plot
would work.
Let me know if you have any ideas.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe something like that, or I'd also be open to discussing whether to support matplotlib plotting functions from our plotting API with a clear distinction that they are not the default hvplot
functions, but that's where my thoughts are not in any maturity as of now. Thinking out loud: Maybe through a newer accessor such as ux.mpl_plot
, or through the existing one with clear function namings such as ux.plot.mpl_edges()
, or something else. I am curious what you all would think about these.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
matplotlib native support and seperate from hvplot sounds good and clearer and more maintainable.
Overview
UxGrid.plot_edges
Expected Usage
PR Checklist
General
Testing
Documentation
Docstrings have updated with any function changesInternal functions have a preceding underscore (_
) and have been added todocs/internal_api/index.rst
docs/api.rst
Examples
docs/examples/
folderdocs/examples.rst
toctreedocs/gallery.yml
with appropriate thumbnail photo indocs/_static/thumbnails/