Make GridLiner into an Artist #2249

rcomer · 2023-09-23T09:26:10Z

Rationale

Thinking about #2246 (comment), it is not intuitive to me that Gridliner._draw_gridliner should be called as part of finding the GeoAxes's bbox, since this method does not seem to update the GeoAxes's position. I tried moving the call so it's only used at draw and not for finding the bbox, and the only test that failed uses fig.tight_layout() before the first draw. Clearly tight_layout cannot create space for artists that do not yet exist.

I wonder if a better approach is to have the GridLiner itself be an artist, then it can interact with the Matplotlib layouts and draw machinery on its own terms. This PR is my attempt at that. I went looking at the container artists in Matplotlib's offsetbox.py for ideas about how to do it. The LineCollection and Text objects are no longer added to the axes directly, but are children of the Gridliner.

Fixes #2246. Also closes #1658 as we can now remove the GridLiner using the standard Artist.remove method. I can add tests for those but I wanted to get some feedback on this first in case I have gone completely off piste!

This also seems to help #2247 - I only see the busy kernel when I interact with the plot.

Implications

If user code is relying on the fact that the LineCollection and Text objects were added directly to the axes, I guess this will break that.

rcomer · 2023-09-23T09:28:05Z

lib/cartopy/mpl/gridliner.py

@@ -436,10 +441,6 @@ def __init__(self, axes, crs, draw_labels=False, xlocator=None,
        self._drawn = False
        self._auto_update = auto_update

-        # Check visibility of labels at each draw event
-        # (or once drawn, only at resize event ?)
-        self.axes.figure.canvas.mpl_connect('draw_event', self._draw_event)


I don't understand why this was here. I think it's telling Matplotlib to call the method on each draw, but that was also happening by calling it within GeoAxes.draw. So was this redundant or am I misunderstanding what this is for?

If I just remove this line from main, the tests pass. Also just removing this seems to account for the improvement to #2247.

What you're saying intuitively makes sense.

rcomer · 2023-09-23T15:41:38Z

Thinking about #2246 (comment), it is not intuitive to me that Gridliner._draw_gridliner should be called as part of finding the GeoAxes's bbox, since this method does not seem to update the GeoAxes's position.

OK I've now realised that the axes' tightbbox incorporates all of its artists, so it does make sense to call _draw_gridliner there. But that leads me to matplotlib/matplotlib#26899

greglucas

This seems like a great approach.

We do have this open issue with another idea for gridlines as well if you wanted to look more into that approach: #1773
But I think it would be more effort than this and not exclusive of this either.

For a test you could add a mock draw method and count the number of draws you get for the gridlines.

greglucas · 2023-09-23T17:15:40Z

lib/cartopy/mpl/gridliner.py

+        # We do not want the labels clipped to axes.
+        self.set_clip_on(False)


Do you want to set the clipping differently for the lines vs the text? Allow the text to be outside, but the lines to be clipped?

Edit: I looked at the code below and I think you are accounting for this after all!

Yes, the lines need clipping to the axes patch, which happens by default within add_collection
https://github.com/matplotlib/matplotlib/blob/ff552f45f2d30c76120fe0355306afac9a9dd2da/lib/matplotlib/axes/_base.py#L2255-L2256

I got a lot of image test failures before I realised that!

greglucas · 2023-09-23T17:16:30Z

lib/cartopy/mpl/gridliner.py

@@ -436,10 +441,6 @@ def __init__(self, axes, crs, draw_labels=False, xlocator=None,
        self._drawn = False
        self._auto_update = auto_update

-        # Check visibility of labels at each draw event
-        # (or once drawn, only at resize event ?)
-        self.axes.figure.canvas.mpl_connect('draw_event', self._draw_event)


What you're saying intuitively makes sense.

greglucas · 2023-09-23T17:17:53Z

lib/cartopy/mpl/gridliner.py

-        # Clear lists of artists
-        for lines in [*self.xline_artists, *self.yline_artists]:
-            lines.remove()
+        # Clear lists of child artists


It would be nice if we could get rid of this as well and just update the artists that are already there instead. Something to look into the future I think.

rcomer · 2023-09-26T16:13:54Z

OK, I've added some tests. Against main the new tests give me

FAILED lib/cartopy/tests/mpl/test_gridliner.py::test_gridliner_count_draws - AssertionError: Expected '_draw_gridliner' to have been called once. Called 2 times.
FAILED lib/cartopy/tests/mpl/test_gridliner.py::test_gridliner_remove - AttributeError: 'Gridliner' object has no attribute 'remove'
FAILED lib/cartopy/tests/mpl/test_gridliner.py::test_gridliner_save_tight_bbox - AttributeError: 'NoneType' object has no attribute 'dpi'

I also

moved the add_artist call into the ax.gridlines() method as I think that's more consistent with mpl plotting methods
got rid of the ax._gridliners attribute as these are now tracked through ax.artists
updated a couple of geoaxes method docstrings to reflect the existing changes

rcomer · 2023-09-26T16:17:20Z

Test failures all seem to be something to do with MODIS.

rcomer · 2023-09-26T16:25:24Z

lib/cartopy/tests/mpl/test_gridliner.py

+        mocked.assert_called_once()
+
+
+def test_gridliner_remove():


I wanted a check_figures_equal for this one to show that adding and removing the gridliner gives the same as not adding it, but that doesn't seem to exist [yet] in pytest-mpl. Another option would be an image test against an empty rectangle...

Can you test against an already existing image?
https://github.com/SciTools/cartopy/blob/main/lib/cartopy/tests/mpl/baseline_images/mpl/test_mpl_integration/simple_global.png

I can try - can you specify a different directory in the image test decorator? Or should I just create a symlink from the gridliner baseline image directory to the target image?

I have no idea... I was just pointing you at an empty figure to test against :)

FWIW, I think your current version with the asserts is fine.

Hmmm. I can report that the decorator takes a baseline_dir keyword, but it doesn't appear to have any effect.

Ah, it's possible if we modify conftest.py. I've put that in a separate commit in case we decide that's not a good idea...

lgolston · 2023-09-26T18:10:39Z

Test failures all seem to be something to do with MODIS.

This issue has been bumped recently at #geopython/OWSLib#769

dopplershift · 2023-09-26T19:31:09Z

This does seem like a good approach.

rcomer · 2023-09-29T14:12:50Z

lib/cartopy/mpl/gridliner.py

+    def get_window_extent(self, renderer=None):
+        self._draw_gridliner(renderer=renderer)
+        visible_text_extents = [c.get_window_extent(renderer=renderer)
+                                for c in self.label_artists if c.get_visible()]


Although this works for all tested cases, it feels a bit wrong to be excluding the lines from the bbox calculation here. Would it be better to define get_tightbbox directly as a union of the tightbboxes of all the child artists?

I think that makes sense to get the union of all child artists as well because they are a part of this artist.

One of the other things I noticed with this when testing it out is if you pan up, the title moves locations based on the axis labels box on the y-axis, which doesn't happen in plain MPL. If the text for the axis extends beyond the spine, then the title gets pushed up with it. Then when the label disappears the title jumps back down again. So I'm wondering if this also needs some adjustment on that aspect as well... (example: #2246)

I can reproduce the oscillating title with main and with v0.22. The culprit is this method:

cartopy/lib/cartopy/mpl/geoaxes.py

Lines 533 to 566 in f2bb81d

def _update_title_position(self, renderer):

super()._update_title_position(renderer)

if not self._gridliners:

return

if self._autotitlepos is not None and not self._autotitlepos:

return

# Get the max ymax of all top labels

top = -1

for gl in self._gridliners:

if gl.has_labels():

for label in (gl.top_label_artists +

gl.left_label_artists +

gl.right_label_artists):

# we skip bottom labels because they are usually

# not at the top

bb = label.get_tightbbox(renderer)

top = max(top, bb.ymax)

if top < 0:

# nothing to do if no label found

return

yn = self.transAxes.inverted().transform((0., top))[1]

if yn <= 1:

# nothing to do if the upper bounds of labels is below

# the top of the axes

return

# Loop on titles to adjust

titles = (self.title, self._left_title, self._right_title)

for title in titles:

x, y0 = title.get_position()

y = max(1.0, yn)

title.set_position((x, y))

I added a commit that addresses the case above: we check whether the horizontal extents of the left/right labels overlap the horizontal extent of the axes. If not, the labels do not affect the title position. I'm not sure whether this is too simplistic or too projection-specific though.

We do not seem to have any existing test coverage for this title positioning method. So I tried this example from when the code went in: #1117 (comment) The good news is that I get identical results with my branch and with main. The bad news is I don't get what was shown in that comment. Note the titles for AlbersEqualArea and LambertConformal are lower here than they were then. A closer look at the AlbersEqualArea example shows that it has no "left", "right" or "top" labels, only "bottom" and "geo". So it seems like "geo" needs to be incorporated into the method somehow. I haven't really got my head around what the "geo" labels do though.

OK, I think I get it now: the left and right labels refer to anything on the vertical spines of the standard rectangular axes. The "geo" labels are everywhere where the spine curves round (I think). This was presumably not the case when the method was originally written. If I’ve understood correctly, we now only need to consider the “top” and “geo” labels when updating the title position. That example now gives me the below image, and I have pulled some of it out into a new image test.

greglucas

I think this is a good improvement.

greglucas · 2023-10-01T14:17:52Z

lib/cartopy/mpl/geoaxes.py

        if self._autotitlepos is not None and not self._autotitlepos:
            return


Minor, but possibly put this check first to short-circuit out of it before iterating through artists?

greglucas · 2023-10-01T14:27:45Z

lib/cartopy/tests/mpl/test_gridliner.py

+        for artist in ax.artists:
+            if isinstance(artist, Gridliner):
+                assert hasattr(artist, '_drawn') and artist._drawn


I realize this was not your doing, but this seems fragile if something changes types and we don't hit this code path. Could we change it to a count assertion num_gridliners_drawn == expected_number to make sure we are hitting the code path we expect?

rcomer · 2023-10-01T16:06:40Z

Thanks @greglucas - I have addressed those two points and also updated a comment that I missed before.

greglucas · 2023-10-02T14:21:42Z

All test failures are due to external WMTS server failures and are unrelated to this PR.

rcomer commented Sep 23, 2023

View reviewed changes

greglucas reviewed Sep 23, 2023

View reviewed changes

rcomer force-pushed the gridliner-updates branch from 8528071 to cda79ad Compare September 26, 2023 16:06

rcomer marked this pull request as ready for review September 26, 2023 16:17

rcomer commented Sep 26, 2023

View reviewed changes

rcomer force-pushed the gridliner-updates branch from cda79ad to 42cd8a0 Compare September 26, 2023 17:14

rcomer mentioned this pull request Sep 28, 2023

Gridliner: don't destroy and recreate artists #2252

Merged

rcomer commented Sep 29, 2023

View reviewed changes

rcomer marked this pull request as draft September 29, 2023 18:08

rcomer force-pushed the gridliner-updates branch 2 times, most recently from c1d042b to 1367d8b Compare September 30, 2023 19:10

rcomer marked this pull request as ready for review September 30, 2023 19:18

greglucas approved these changes Oct 1, 2023

View reviewed changes

greglucas added the Component: Gridlines label Oct 1, 2023

greglucas added this to the Next Release milestone Oct 1, 2023

rcomer added 3 commits October 1, 2023 17:01

Make Gridliner into an Artist

4f87b1c

Image test for gridline removal

0ce1370

FIX: title repositioning logic

d3a3249

rcomer force-pushed the gridliner-updates branch from 1367d8b to d3a3249 Compare October 1, 2023 16:05

rcomer mentioned this pull request Oct 2, 2023

Using Gridliner and Figure(layout="constrained") together misbehaves #2245

Closed

greglucas merged commit e424784 into SciTools:main Oct 2, 2023
1 of 11 checks passed

rcomer deleted the gridliner-updates branch October 6, 2023 18:06

rcomer mentioned this pull request Feb 20, 2024

Calling tight_layout between gridlines and set_extent screws up #1792

Closed

rcomer mentioned this pull request May 23, 2024

GeoAxes._update_title_position called before Gridliner._labels is populated #2390

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make GridLiner into an Artist #2249

Make GridLiner into an Artist #2249

rcomer commented Sep 23, 2023 •

edited

Loading

rcomer Sep 23, 2023 •

edited

Loading

greglucas Sep 23, 2023

rcomer commented Sep 23, 2023

greglucas left a comment

greglucas Sep 23, 2023

rcomer Sep 24, 2023

greglucas Sep 23, 2023

greglucas Sep 23, 2023

rcomer commented Sep 26, 2023

rcomer commented Sep 26, 2023

rcomer Sep 26, 2023

greglucas Sep 26, 2023

rcomer Sep 26, 2023

greglucas Sep 26, 2023

rcomer Sep 26, 2023

rcomer Sep 26, 2023

lgolston commented Sep 26, 2023

dopplershift commented Sep 26, 2023

rcomer Sep 29, 2023 •

edited

Loading

greglucas Sep 29, 2023

rcomer Sep 29, 2023

rcomer Sep 30, 2023 •

edited

Loading

rcomer Sep 30, 2023 •

edited

Loading

greglucas left a comment

greglucas Oct 1, 2023

greglucas Oct 1, 2023

rcomer commented Oct 1, 2023

greglucas commented Oct 2, 2023

		# We do not want the labels clipped to axes.
		self.set_clip_on(False)

	def _update_title_position(self, renderer):
	super()._update_title_position(renderer)
	if not self._gridliners:
	return

	if self._autotitlepos is not None and not self._autotitlepos:
	return

	# Get the max ymax of all top labels
	top = -1
	for gl in self._gridliners:
	if gl.has_labels():
	for label in (gl.top_label_artists +
	gl.left_label_artists +
	gl.right_label_artists):
	# we skip bottom labels because they are usually
	# not at the top
	bb = label.get_tightbbox(renderer)
	top = max(top, bb.ymax)
	if top < 0:
	# nothing to do if no label found
	return
	yn = self.transAxes.inverted().transform((0., top))[1]
	if yn <= 1:
	# nothing to do if the upper bounds of labels is below
	# the top of the axes
	return

	# Loop on titles to adjust
	titles = (self.title, self._left_title, self._right_title)
	for title in titles:
	x, y0 = title.get_position()
	y = max(1.0, yn)
	title.set_position((x, y))

Make GridLiner into an Artist #2249

Make GridLiner into an Artist #2249

Conversation

rcomer commented Sep 23, 2023 • edited Loading

Rationale

Implications

rcomer Sep 23, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcomer commented Sep 23, 2023

greglucas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcomer commented Sep 26, 2023

rcomer commented Sep 26, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgolston commented Sep 26, 2023

dopplershift commented Sep 26, 2023

rcomer Sep 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcomer Sep 30, 2023 • edited Loading

Choose a reason for hiding this comment

rcomer Sep 30, 2023 • edited Loading

Choose a reason for hiding this comment

greglucas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rcomer commented Oct 1, 2023

greglucas commented Oct 2, 2023

rcomer commented Sep 23, 2023 •

edited

Loading

rcomer Sep 23, 2023 •

edited

Loading

rcomer Sep 29, 2023 •

edited

Loading

rcomer Sep 30, 2023 •

edited

Loading

rcomer Sep 30, 2023 •

edited

Loading