-
Notifications
You must be signed in to change notification settings - Fork 422
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
Fixes for Cartopy 0.18 #1372
Conversation
I think the fix for |
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.
ba27d89
to
9999d76
Compare
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. |
2822b84
to
2625e99
Compare
0.18 started autoscaling map features and updated some gridline handling, causing changes to our test images.
2625e99
to
2fe93c2
Compare
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... |
2fe93c2
to
cf9e003
Compare
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.
894f15f
to
12c7582
Compare
Ok, we're close now. A few outstanding failures to address:
|
After doing some |
12c7582
to
508948a
Compare
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.
Tripped doc8.
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.
508948a
to
5c00528
Compare
@jthielen It looks like those tests don't exist on the |
@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. |
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? |
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. |
Correct. So would a separate PR to master (which you can then cherry pick commits to 0.12.x) be good? |
@dopplershift Were all the fixes here incorporated in #1416, and if so, can this be closed? |
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. |
Description Of Changes
MetPyMapFeature
. We really don't need to be inheritingNaturalEarthFeature
, we're pretty much overriding the only thing it does (read and return geometries), so just inheritFeature
.Checklist