-
Notifications
You must be signed in to change notification settings - Fork 94
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
Fix compatibility with cartopy 0.20.0+ #378
Conversation
Codecov Report
@@ Coverage Diff @@
## main #378 +/- ##
==========================================
- Coverage 93.54% 93.40% -0.14%
==========================================
Files 50 50
Lines 10436 10405 -31
==========================================
- Hits 9762 9719 -43
- Misses 674 686 +12
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changes lgtm, however, the new cartopy 0.20 isn't tested, right?
It is tested. That's how I knew that things weren't working (my other PRs were failing). This does not explicitly test older versions of cartopy, but I think given the improvements that are going on in cartopy we push users to update to newer cartopy as soon as possible. I'm not saying that old cartopy versions won't work with pyresample, but I would consider them on a short support cycle for the pyresample functionality. We can remove a lot of code from pyresample after my PR in cartopy gets merged and released. Here's the conda log from one of the ubuntu jobs run here:
Otherwise, you requested changes? |
OK, I don't understand the how you can test that both the new and old cartopy support is working without changing the tests more... |
As described on slack, it isn't really possible to test both old and new cartopy. Eventually we want to deprecate old cartopy version support so we (@mraspaud and I) decided it would be easier to do it now. This PR now fixes support for 0.20.0+ but removes support for old versions. This cleans out a lot of old code. If my new cartopy PR gets merged and released then we could remove even more code from pyresample and just use cartopy directly. |
@mraspaud I just found out from a student employee that the URL that cartopy uses for downloading shapefiles from NaturalEarth was updated for 0.20.0. The old one doesn't work any more. So forcing users to update to 0.20.0 doesn't sound so bad. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Cartopy 0.20.0 includes updates by @snowman2 so that cartopy's CRS objects are now a subclass of pyproj's CRS class. Somehow this release broke compatibility with pyresample's hacky cartopy conversion where we started getting errors in our tests related to
towgs84
PROJ.4 string parameters being invalid. Something got lost in the CRS -> proj4 dict -> proj4 str -> CRS conversion.Rather than figure out where this conversion issue was happening, I chose to implement something that I discussed in the cartopy issue here (SciTools/cartopy#813). Instead of tricking cartopy into accepting arbitrary PROJ.4 information, this uses the new pyproj/cartopy CRS functionality to pass pyresample's pyproj CRS object directly to cartopy. This PR acts as a test for this functionality being added directly to cartopy (see referenced issue).