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

Parametrize inline gridliner tests #1900

Merged
merged 2 commits into from
Oct 7, 2021

Conversation

QuLogic
Copy link
Member

@QuLogic QuLogic commented Oct 6, 2021

Rationale

Unlike the old ImageTesting decorator, pytest-mpl support parametrized tests. While this does add a bunch of new tests/images, they are all far smaller than the single test, and can be run in parallel.

Implications

The overhead of all the individual images is only ~6% bigger than the two mega test images.

Previously, our 5 slowest gridliner tests were:

104.48s call     lib/cartopy/tests/mpl/test_gridliner.py::test_grid_labels_inline
79.11s call     lib/cartopy/tests/mpl/test_gridliner.py::test_grid_labels_inline_usa
13.77s call     lib/cartopy/tests/mpl/test_gridliner.py::test_grid_labels
9.77s call     lib/cartopy/tests/mpl/test_gridliner.py::test_gridliner
8.42s call     lib/cartopy/tests/mpl/test_gridliner.py::test_grid_labels_tight

and now they are:

16.03s call     lib/cartopy/tests/mpl/test_gridliner.py::test_grid_labels_inline[NorthPolarStereo]
16.01s call     lib/cartopy/tests/mpl/test_gridliner.py::test_grid_labels_inline[SouthPolarStereo]
12.15s call     lib/cartopy/tests/mpl/test_gridliner.py::test_grid_labels
11.54s call     lib/cartopy/tests/mpl/test_gridliner.py::test_grid_labels_inline[Stereographic]
10.05s call     lib/cartopy/tests/mpl/test_gridliner.py::test_grid_labels_inline[Miller]

But instead of getting stuck on one single test for almost 2 minutes, we can run in 4-way parallel at about 70.17s.

Since they are all generated anew, I switched to the new mpl20 style. Besides the style, I believe this causes the following changes to gridline locations for inline:

  • EuroPP labels are half the tick interval as before
  • OSNI labels latitude every 0.5° instead of every 1°

and for inline USA:

  • InterruptedGoodeHomolosine labels longitude every 10° instead of every 20°
  • LambertConformal labels longitude every 10° instead of every 20°
  • NorthPolarStereo labels latitude every 5° instead of every 10°
  • Robinson labels longitude every 10° instead of every 20°
  • Stereographic labels longitude every 10° instead of every 20°

The automatic display of labels has a few small differences in the inline results, probably due to different Axes size:

  • Geostationary labels 20°W and 40°E instead of 0°
  • LambertAzimuthalEqualArea also labels 30°N/30°S somehow
  • LambertConformal also labels the inner 60°W
  • NearsidePerspective labels 40°E instead of 60°E
  • Orthographic also labels 0° longitude
  • OSGB also labels 6°W and 2°W on the top, and 4°W on the bottom
  • Robinson also labels 60°W

For the inline USA results, there are fewer differences:

  • AzimuthalEquidistant also labels 60°N, but somewhat poorly
  • EuroPP, OSGB, and OSNI are skipped since their extent is locked
  • Geostationary also labels 60°W on the bottom
  • Gnomonic also labels 68°W
  • NearsidePerspective also labels 60°W on the bottom
  • Orthographic also labels 70°W on the bottom

@QuLogic QuLogic added this to the 0.21 milestone Oct 6, 2021
@QuLogic
Copy link
Member Author

QuLogic commented Oct 6, 2021

All images generated with Matplotlib 3.5.0rc1, so we'll have to see if they need tolerance increases.

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.

Overall I think this is a really nice improvement. Now if a projection is changed, only that one image would need to be updated.

When you are listing out the updates to the images and say that now the labels are every 10 degrees instead of 20. Has it been that way for a while and our tolerances were just so large that we didn't bother updating the images? Or, is it that the default style updates the locators?

lib/cartopy/tests/mpl/test_gridliner.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
@QuLogic
Copy link
Member Author

QuLogic commented Oct 7, 2021

When you are listing out the updates to the images and say that now the labels are every 10 degrees instead of 20. Has it been that way for a while and our tolerances were just so large that we didn't bother updating the images? Or, is it that the default style updates the locators?

Changing the style changes the locator. Just rebuilding with the classic style only has the second set of changes (plus or minus a few small ones).

@QuLogic
Copy link
Member Author

QuLogic commented Oct 7, 2021

Without titles, the new set of images appears to be smaller even than the two originals.

Unlike the old `ImageTesting` decorator, pytest-mpl support parametrized
tests. While this does add a bunch of new tests/images, they are all far
smaller than the single test, and can be run in parallel.

Since they are all generated anew, I switched to the new mpl20 style.
Besides the style, I believe this causes the following changes to
gridline locations for inline:

- EuroPP labels are half the tick interval as before
- OSNI labels latitude every 0.5° instead of every 1°

and for inline USA:

- InterruptedGoodeHomolosine labels longitude every 10° instead of
  every 20°
- LambertConformal labels longitude every 10° instead of every 20°
- NorthPolarStereo labels latitude every 5° instead of every 10°
- Robinson labels longitude every 10° instead of every 20°
- Stereographic labels longitude every 10° instead of every 20°

The automatic display of labels has a few small differences in the
inline results, probably due to different Axes size:

- Geostationary labels 20°W and 40°E instead of 0°
- LambertAzimuthalEqualArea also labels 30°N/30°S somehow
- LambertConformal also labels the inner 60°W
- NearsidePerspective labels 40°E instead of 60°E
- Orthographic also labels 0° longitude
- OSGB also labels 6°W and 2°W on the top, and 4°W on the bottom
- Robinson also labels 60°W

For the inline USA results, there are fewer differences:

- AzimuthalEquidistant also labels 60°N, but somewhat poorly
- EuroPP, OSGB, and OSNI are skipped since their extent is locked
- Geostationary also labels 60°W on the bottom
- Gnomonic also labels 68°W
- NearsidePerspective also labels 60°W on the bottom
- Orthographic also labels 70°W on the bottom
def test_grid_labels_inline_usa():
print(plt.get_backend())
@pytest.mark.parametrize('proj', TEST_PROJS)
@pytest.mark.mpl_image_compare(style='mpl20', tolerance=0.79)
Copy link
Member Author

Choose a reason for hiding this comment

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

Only really NorthPolarStereo fails, but this is pretty close to 0.5, so I just did it for everything.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should this only be for the MPL < 3.5 cases?

Copy link
Member Author

Choose a reason for hiding this comment

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

Could be, but it was close enough to the default 0.5 that maybe it's okay everywhere?

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Much better than the previous 40+ on some of the images :) I think breaking these up into smaller tests will also help to not grow the tolerances quite as much arbitrarily too.

Tis fixes test on minimum Matplotlib
@greglucas
Copy link
Contributor

Some of the label positioning does look awkward, like in the Geostationary choosing 40, 20, 60, instead of centering on 0. But you aren't doing anything in this, that is all automatically being chosen. I think those "defaults" can be improved in follow-up PRs.

@greglucas greglucas merged commit 0caad2b into SciTools:master Oct 7, 2021
@QuLogic QuLogic deleted the parametrized-grid-tests branch October 7, 2021 20:29
@dopplershift dopplershift mentioned this pull request Jun 28, 2022
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.

2 participants