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

Include gridliner labels in tight bbox calculation #1355

Merged
merged 6 commits into from
Jul 1, 2020

Conversation

lukelbd
Copy link
Contributor

@lukelbd lukelbd commented Aug 16, 2019

This PR replaces #1354.

Rationale

matplotlib.axes.Axes.get_tightbbox fails to include cartopy gridliner labels. This is because cartopy.mpl.gridliner.Gridliner._draw_gridliner is not called until Axes.draw is invoked, which comes after Figure.draw invokes Figure.tight_layout.

This change adds a get_tightbbox override to the GeoAxes class, which calls _draw_gridliner on all the GridLiner objects, so the tight layout algorithm will work. This mimics the approach used in the matplotlib API: Axes.draw and Axes.get_tightbbox contain many identical calls to various post-processing steps, like _update_title_position.

Implications

You can now use fig.tight_layout() on figures with cartopy axes containing gridliner labels.

Examples

I wasn't able to install a development version of cartopy, but I implemented a virtually identical change in a GeoAxes subclass in my proplot package (proplot calls fig.tight_layout() on all figures by default).

Below, the first example is without the get_tightbbox override (there are labels present, they are just off the edge of the figure). The second example is with the get_tightbbox override.

import proplot as plot
fig, ax = plot.subplots(proj='pcarree', axwidth=3)
ax.format(labels=True, lonlines=60, latlines=30, title='Plate Carrée')
fig.save('example.png')

A similar pyplot example could be generated by just calling fig.tight_layout() on any cartopy plot with gridlines.

example1
example2

@SciTools-assistant SciTools-assistant added the Blocked: CLA needed See https://scitools.org.uk. Submit the form at: https://scitools.org.uk/cla/v4/form label Aug 16, 2019
Copy link
Contributor

@dopplershift dopplershift left a comment

Choose a reason for hiding this comment

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

In general, this looks good. I have one question, and then would you be willing to add your sample as a test?

lib/cartopy/mpl/geoaxes.py Outdated Show resolved Hide resolved
@dopplershift dopplershift added this to the 0.18 milestone Aug 16, 2019
@greglucas
Copy link
Contributor

@lukelbd did you sign the CLA? Could you rebase if so to rerun CI on this.

@lukelbd
Copy link
Contributor Author

lukelbd commented Mar 5, 2020

@greglucas Sorry for the delay; I've signed the CLA, and can try to add a test by next weekend.

@SciTools-assistant SciTools-assistant removed the Blocked: CLA needed See https://scitools.org.uk. Submit the form at: https://scitools.org.uk/cla/v4/form label Mar 5, 2020
@greglucas
Copy link
Contributor

Sounds good, @lukelbd! @dopplershift was there anything else you were requesting in addition to the test?

@dopplershift
Copy link
Contributor

@greglucas I think at this point it just needs a test.

@greglucas
Copy link
Contributor

ping @lukelbd.

@greglucas
Copy link
Contributor

@lukelbd we are going to try and push another beta release out this weekend. Do you think you can add the test by then, or should we look to push this back to the next 0.19 release milestone?

@QuLogic QuLogic modified the milestones: 0.18, 0.19 Apr 27, 2020
@QuLogic
Copy link
Member

QuLogic commented Apr 27, 2020

Tagging the rc today, so I'm pushing this back to 0.19.

@greglucas
Copy link
Contributor

I just ran into the case of saving a pdf and the side grid labels were partially cut off. I think this may solve that issue...? It would be good to get it into the code since it seems logical enough.

@lukelbd
Copy link
Contributor Author

lukelbd commented Jun 12, 2020

Sorry forgot about this PR, was really busy when I was pinged. Will take a look this weekend. I think this will also need to be updated for the 0.18 gridliner API

lib/cartopy/mpl/geoaxes.py Outdated Show resolved Hide resolved
lib/cartopy/mpl/geoaxes.py Outdated Show resolved Hide resolved
lib/cartopy/mpl/geoaxes.py Outdated Show resolved Hide resolved
@lukelbd
Copy link
Contributor Author

lukelbd commented Jun 15, 2020

I added a test for the "latest everything" build only. Turns out matplotlib 1.5.1 does not use Figure.get_tightbbox() to calculate the bounding box -- it uses a get_figure_tight_layout() function in tight_layout.py instead. So it is impossible to override the tight layout algorithm:

https://github.com/matplotlib/matplotlib/blob/be91fac9fa2b7250080557e723af75124659da4e/lib/matplotlib/figure.py#L1720-L1756

Update 2020-07-14: The above reasoning was incorrect, but tight layout override does fail in matplotlib 1.5.0. After empirical testing, the tight layout override works only in matplotlib >=3.0. See #1575 for details.

My test is similar to test_grid_labels, but it calls Figure.tight_layout() at the end and ensures that every gridliner was drawn by get_tightbbox() by ensuring Gridliner._plotted == True.

Here is the baseline result:

image

And here is the result without the Figure.tight_layout() call:

result-gridliner_labels_tight

@lukelbd
Copy link
Contributor Author

lukelbd commented Jun 15, 2020

Not sure why it's failing... I built the test image by running pytest as follows:

conda config --set always_yes yes --set changeps1 no --set show_channel_urls yes
conda config --add channels conda-forge
conda config --add channels conda-forge/label/testing
conda config --set restore_free_channel true

export CYTHON_COVERAGE=1
echo "backend: agg" > ~/.matplotlib/matplotlibrc  # macOS matplotlibrc

conda create -n cartopy-dev python=3.6 cython numpy matplotlib-base proj4 pykdtree scipy fiona pillow pytest pytest-xdist filelock pep8 pyshp shapely six requests pyepsg owslib pytest-cov coveralls
conda activate cartopy-dev

pip install --no-deps -e .
python -c "import cartopy; print('Version ', cartopy.__version__)" && python setup.py version
pytest

@lukelbd
Copy link
Contributor Author

lukelbd commented Jun 15, 2020

Nevermind -- looks like the latest push fixed it! I might have accidentally generated the baseline image without the backend: agg line or in the matplotlib 1.5.1 environment.

Anything else I need to do?

Copy link
Contributor

@greglucas greglucas left a comment

Choose a reason for hiding this comment

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

Thanks for adding the image test! Overall I think this looks good. Just a few minor comments.

lib/cartopy/mpl/geoaxes.py Outdated Show resolved Hide resolved
lib/cartopy/tests/mpl/test_gridliner.py Outdated Show resolved Hide resolved
lib/cartopy/tests/mpl/test_gridliner.py Outdated Show resolved Hide resolved
@lukelbd
Copy link
Contributor Author

lukelbd commented Jun 17, 2020

@greglucas Done!

@greglucas greglucas requested a review from dopplershift June 17, 2020 13:15
Copy link
Contributor

@greglucas greglucas left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@lukelbd
Copy link
Contributor Author

lukelbd commented Jun 18, 2020

@dopplershift I think the remaining requested change is out of date

@dopplershift dopplershift merged commit 4703efc into SciTools:master Jul 1, 2020
@lukelbd lukelbd deleted the tightbbox-fix branch July 1, 2020 04:57
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.

6 participants