Skip to content

Conversation

nils3er
Copy link

@nils3er nils3er commented Oct 13, 2025

Overview

  • Introduces UxGrid.plot_edges
  • extracts visible edges for the extract in an axis.
    • not exact. A edge is plotted if one of the nodes is in the extent.
  • Adds a test
  • Adds an example to the user guide

Expected Usage

import uxarray as ux

grid_path = "/path/to/grid.nc"

uxgrid = ux.open_grid(grid_path)

center = -50, 0
fig, ax = plt.subplots(
    1,
    1,
    constrained_layout=True,
    subplot_kw={"projection": ccrs.Orthographic(*center)},
)
ax.add_feature(cfeature.LAND)
ax.add_feature(cfeature.COASTLINE)
ax.set_extent([center[0]+20, center[0]-20, center[1]-10, center[1]+10])

uxgrid.plot_edges(ax=ax, color="blue")
plt.show()

PR Checklist

General

  • An issue is linked created and linked
  • Add appropriate labels
  • Filled out Overview and Expected Usage (if applicable) sections

Testing

  • Adequate tests are created if there is new functionality
  • Tests cover all possible logical paths in your function
  • Tests are not too basic (such as simply calling a function and nothing else)
    • Could be improved with a local axis

Documentation

  • Docstrings have been added to all new functions
  • Docstrings have updated with any function changes
  • Internal functions have a preceding underscore (_) and have been added to docs/internal_api/index.rst
  • User functions have been added to docs/api.rst

Examples

  • Any new notebook examples added to docs/examples/ folder
  • Clear the output of all cells before committing
  • New notebook files added to docs/examples.rst toctree
  • New notebook files added to new entry in docs/gallery.yml with appropriate thumbnail photo in docs/_static/thumbnails/

Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@rajeeja
Copy link
Contributor

rajeeja commented Oct 15, 2025

@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:

  1. What was your specific use case that the existing uxgrid.plot.edges() couldn't handle?
  2. Did you try using the existing method with backend='matplotlib' first

@nils3er
Copy link
Author

nils3er commented Oct 16, 2025

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.
The reason for this is that matplotlib is very slow in adding all the lines to the plot even if the lines are out of the extent and not visible at all. We encountered this problem actually before moving to uxarray and solved it by filtering out the non-visible edges. I simple translated this solution then to out uxarray implementation. This speeds up the example to 400ms. This led this this PR now, because I thought it would be valuable for others as well.

Unfortunately I don't managed to get the grid.plot.edges function working outside of jupyter. I'm not very experienced with hvplot, but as far as i can see its not possible to pass a cartopy.GeoAxes to it, which would be necessary in our case.

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.

@rajeeja
Copy link
Contributor

rajeeja commented Oct 16, 2025

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

@erogluorhan
Copy link
Member

@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 ux.plot accessor that itself can be called and also allows specialized functions to be called, e.g. ux.plot.edges() as Rajeev mentioned. All of these default to hvplot. So far, whenever we needed to provide user with functionality or data structures that would be ingested by the matplotlib chain (including cartopy of course), we didn't do it through the .plot accessor, rather we provided convenience functions, e.g. ux.UxDataArray.to_raster().

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 hvplot, we'll need to re-consider things.

@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?

@philipc2 philipc2 self-requested a review October 17, 2025 03:34
Comment on lines +2381 to +2385
def plot_edges(
self,
ax: Optional["cartopy.mpl.geoaxes.GeoAxes"] | None, # noqa: F821
**plot_kwargs,
):
Copy link
Member

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.

Copy link
Member

@erogluorhan erogluorhan Oct 17, 2025

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.

@rajeeja rajeeja self-requested a review October 17, 2025 16:32
Copy link
Contributor

@rajeeja rajeeja left a 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.

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.

4 participants