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

Fixes for Cartopy 0.18 #1372

Closed
wants to merge 13 commits into from
Closed

Conversation

dopplershift
Copy link
Member

Description Of Changes

  • Fix implementation of MetPyMapFeature. We really don't need to be inheriting NaturalEarthFeature, we're pretty much overriding the only thing it does (read and return geometries), so just inherit Feature.
  • Update tests (for changes to default map boundaries and gridlines)
  • TODO: Fix clipping issue

Checklist

@dopplershift dopplershift requested a review from dcamron as a code owner May 13, 2020 05:42
@dopplershift dopplershift added this to the 0.12.2 milestone May 13, 2020
@dopplershift
Copy link
Member Author

I think the fix for MetPyMapFeature here works as a permanent fix and is limited enough for 0.12.2.

CartoPy 0.18 started validating scales for NaturalEarthFeature, which
breaks with the scales for our own map features. There's really no
reason to inherit from NaturalEarthFeature, so just make this inherit
Feature and add a few missing things.
Matplotlib (as of 3.2.2) does not properly clip text to paths. For now,
add a clipbox for the Axes bounding box, which will supplement the set
clip path.
@dopplershift
Copy link
Member Author

Thanks @kgoebber for tracking down the clipping issue. After a couple hours working through things, it turns out that single line is the best way to do this in a future-proof way--until a fix for the matplotlib bug gets in.

0.18 started autoscaling map features and updated some gridline
handling, causing changes to our test images.
@dopplershift
Copy link
Member Author

Ok, updated a few more image thresholds for tests that had no discernible change in my linux VM. Also fixed some lint as well as a test that was checking for a numpy warning--and failed with 1.19. Let's see how this goes now...

@dopplershift
Copy link
Member Author

Now have fixed doctests (pint prints fewer digits) and have also updated for Matplotlib 3.3.0rc1. Let's see if we can get pint to install from git again...because of course that has also broken.

Matplotlib <3.3 checks for closed contours using floating point
equality. For *this test*, this was causing different results on macOS
vs. Windows/Linux. Fixing this actually changes the labelling of another
closed contour as well.
We were using pytest to catch *and check* a warning coming from numpy,
which we no longer get. Just use a simple warning filter since it's not
part of *our* API anyway.
@dopplershift dopplershift force-pushed the cartopy-0.18 branch 2 times, most recently from 894f15f to 12c7582 Compare July 1, 2020 09:22
@dopplershift
Copy link
Member Author

Ok, we're close now. A few outstanding failures to address:

  • test_declarative_barb_gfs and test_declarative_barb_gfs_knots fail on Matplotlib 3.3.0rc1 due to rotations now being done with 64-bit values. No, I'm not kidding, that causes our image differences
  • test_smooth_window_1d_dataarray and test_gradient_xarray_implicit_axes_transposed fail due to some kind of unit issue with xarray and pint master
  • A bunch of image tests are failing on matplotlib 2.1; not quite sure why yet or why so many would need threshold adjustments

@jthielen
Copy link
Collaborator

jthielen commented Jul 1, 2020

After doing some git bisect, the two xarray with Pint issues (test_smooth_window_1d_dataarray and test_gradient_xarray_implicit_axes_transposed) appeared with pydata/xarray#3847. In short, xr.testing.assert_allclose will now raise a dimensionality error when trying to compare a DataArray with Quantity inside with one with units on the attribute. This will all likely be adjusted in #1353, so I'm not sure what the best interim fix is for 0.12.2...perhaps testing each important piece of the DataArray ourselves?

This is no longer needed since we're on newer matplotlib versions. Also,
now this fixture actually *causes* problems because matplotlib is
relying on builtin round returning an int (when not asking for digits),
which numpy's does not do.
A few items got deprecated/removed and need updating.
Not sure why this started failing on Travis, but it's only a minor
increase. Unable to reproduce locally on a config that should nearly
match Travis.
@dopplershift
Copy link
Member Author

@jthielen It looks like those tests don't exist on the 0.12.x branch, so I'm guessing they came in with some of your work that's only on master? Assuming that's the case (we'll know for sure when I do a PR to update 0.12.x), we can just wait to fix us with pint/xarray master on #1353. The important part is that we've checked what the problems are.

@jthielen
Copy link
Collaborator

jthielen commented Jul 1, 2020

@dopplershift Yes, now that I look back at it, those were added in #1223 and #1260 respectively, so I'll just plan on addressing it in #1353.

@jthielen
Copy link
Collaborator

jthielen commented Jul 9, 2020

In coming back to this, I'll plan on fixing those xarray tests on master in #1378 instead, since that PR comes before #1353. Though, in digging through things now, hgrecco/pint#1123 (recently merged in Pint) is breaking a fair number of our EL/LFC/CAPE/CIN tests in thermo. Where would you want to include a patch for that?

@dopplershift
Copy link
Member Author

Since it's only Pint master (right?), we should be able to get this green without it. All that's waiting on is me to figure out what's going on with 2.1 (I'm just going to update the images for the matplotlib 3.3 RC).

What's weird about matplotlib 2.1 is this used to pass and I don't think this changed, so I was trying to investigate whether they were falsely passing before.

@jthielen
Copy link
Collaborator

jthielen commented Jul 9, 2020

Since it's only Pint master (right?), we should be able to get this green without it.

Correct. So would a separate PR to master (which you can then cherry pick commits to 0.12.x) be good?

@jthielen
Copy link
Collaborator

@dopplershift Were all the fixes here incorporated in #1416, and if so, can this be closed?

@dopplershift
Copy link
Member Author

All of the Cartopy-0.18-relevant fixes were in #1416. There are of course several other things that will go in other PRs, but I don't want to close this until they're all in.

@dopplershift dopplershift marked this pull request as draft July 27, 2020 16:32
@dopplershift
Copy link
Member Author

All of this went in in pieces, among them #1411, #1413, #1416, #1420, and #1422. (I checked by rebasing on current master.)

@dopplershift dopplershift deleted the cartopy-0.18 branch August 5, 2020 05:33
@dopplershift dopplershift removed this from the 0.12.2 milestone Aug 5, 2020
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.

Cartopy 0.18.0 USCOUNTIES '20m' invalid
2 participants