-
Notifications
You must be signed in to change notification settings - Fork 415
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
Use PyProj instead of Cartopy for internal coordinate transforms and rename crs coordinate #1483
Conversation
89a5378
to
85cffcd
Compare
Just fixed up the rebase of this on latest master after the changes of #1490, but the todo list above remains. |
85cffcd
to
011f45a
Compare
I looked at the list on Python 3, and API wise I think we'll be fine. However, they are dropping older versions of PROJ, and doing enough that I think the right approach is to make our PyProj dependency right now |
46873e6
to
be31c7d
Compare
The reason for the test failures is that before for...CartoPy reasons...we were using Before updating all the test values, I was able to make the tests pass by hacking the test metadata to specify a WGS84 spheroid, so that's clearly the issue. Further evidence that shifting over to using PyProj directly for calcs is a good thing to do. |
be31c7d
to
3fdb776
Compare
Bumping our minimum pyproj to 2.3 was enough to make tests pass for me locally. Assuming that holds on CI, let's just go with that (released Aug 2019) rather than trying to do any special work-arounds. |
3fdb776
to
588784c
Compare
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.
I like these changes, and didn't find any other issues myself!
8272a02
to
cab7bd9
Compare
…rename crs coordinate
Was accidentally trying to upload coverage only when we ran without it.
Makes it easier to debug issues while PR is still in development.
Necessary because we are now properly using the Earth ellipsoid that is present in the data for lat/lon projection. Previously, we were using CartoPy's PlateCarree, which doesn't support globes--thus when we got the spheroid we used the CartoPy default, which is WGS84. Was able to reproduce previous test values by overriding data ellipsoid to specify WGS84. Updating the test values is better long-term than putting a WGS84 hack into the data fixture.
Apparently these have been changed for security reasons: https://github.blog/changelog/2020-10-01-github-actions-deprecating-set-env-and-add-path-commands/
cab7bd9
to
038872e
Compare
038872e
to
ee2117d
Compare
Description Of Changes
Implements the changes discussed in #1455 (comment). Part of this was changing the API for
lat_lon_grid_deltas
to take the PyProjGeod
directly rather than by constructor kwargs (makes it easier to handle fromgrid_deltas_from_dataarray
, which extracts theGeod
from the PyProjCRS
).Marking as draft/work-in-progress for now since the following have yet to be done:
azimuth_range_to_lat_lon
and any otherGeod
uses to takegeod
arg rather thankwargs
to construct newGeod
pyproj >= 2.2
a required dependency (unmodified as of now)If someone wanted to take these items on to speed up the process, that would be great! Otherwise, I'll loop back to this after the xarray stuff for 1.0.
Checklist