-
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
Xarray + CF + CartoPy Projection Handling #786
Conversation
examples/xarray_projections.py
Outdated
import matplotlib.pyplot as plt | ||
import xarray as xr | ||
|
||
import metpy.io.xarray # To ensure accessors are available |
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.
F401 'metpy.io.xarray' imported but unused
metpy/io/xarray.py
Outdated
except KeyError: | ||
import warnings | ||
warnings.warn( | ||
'Could not find variable corresponding to the value of grid_mapping: {}'.format( |
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.
E501 line too long (100 > 95 characters)
metpy/io/xarray.py
Outdated
def _fixup_coords(self, var): | ||
for coord_name, data_array in var.coords.items(): | ||
if data_array.attrs.get('standard_name') in ( | ||
'projection_x_coordinate', 'projection_y_coordinate'): |
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.
E122 continuation line missing indentation or outdented
metpy/io/xarray.py
Outdated
except DimensionalityError: # Radians! | ||
new_data_array = data_array.copy() | ||
scaled_vals = new_data_array.units.array * (var.coords['crs'].item()[ | ||
'perspective_point_height'] * units.meters) |
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.
E126 continuation line over-indented for hanging indent
E501 line too long (111 > 95 characters)
Hi @dopplershift did you find a way to read a Grib file? I have been reading anything related to Grib format, decoding it, using struct and Xarray. I have been trying to find a solution to decode in pure python. Looks like you have been looking at the same thing. I want to decode Canadian Grib files as well. I might be using your solution. |
@eliteuser26 I think we'll be implementing something sooner rather than later since we're participating in the efforts for the next GRIB standard. Keep an eye on #411 and any related PRs. This particular PR is focused just on decoding projection information that complies with the netCDF-CF standard into proper CartoPy objects. |
metpy/plots/mapping.py
Outdated
@CFProjection.register('latitude_longitude') | ||
def make_latlon(attrs_dict, globe): | ||
# TODO: 180 here needs to be determined by longitude? | ||
# return ccrs.PlateCarree(globe=globe, central_longitude=180) |
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.
E115 expected an indented block (comment)
Ok, new challenge: (cross ref SciTools/cartopy#709 for
After writing this up, I think I lean towards (3). The downside is that proper use then requires passing There's an additional challenge where CartoPy seems to work poorly with longitudes that are in [0, 360] rather than [-180, 180], but that's a separate issue--and more easily worked around. (ping @deeplycloudy who has some experience in thinking about projections and may be interested in this PR in general.) |
metpy/plots/mapping.py
Outdated
@CFProjection.register('latitude_longitude') | ||
def make_latlon(attrs_dict, globe): | ||
# TODO: 180 here needs to be determined by longitude? | ||
# return ccrs.PlateCarree(globe=globe, central_longitude=180) |
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.
E115 expected an indented block (comment)
Follow up: |
When using openlayers I did come across the issue of using -180 to 180 degree of longitude when displaying polygons. When a point fell outside the 180 degree of longitude boundary it created an artifact where the point was on the other side of the globe. However I did find a solution that resolve this issue by using 0 to 360 degree of longitude east or west. And it worked like a charm. |
@eliteuser26 Right. Another option is to specify the central longitude of the |
I always thought something seemed fishy about the common use of When I think about problems like this I always think about explicitly transforming lon/lat/alt (specified with respect to a globe) to a target coordinate system before plotting, which sounds like (2) above. If you don't have the third coordinate, just set alt=0. It seems CartoPy requires a projection be 2D for it to be drawable, for obvious reasons. However, this behavior causes problems because it is a degenerate way to treat geospatial coordinates - the problem is fundamentally 3D, and includes a vertical coordinate with respect to the plane of the projection. proj.4 handles everything in 3D under the hood, and you can say "my coords are lon/lat/alt and I want to go to this map projection" in one line — and latlong is a valid projection, just like 'lcc' in proj.4.
Why not write a Projection for CartoPy that takes lon/lat/alt and does the right thing with proj.4? One could also amend CartoPy to make the Projection class 3D, and then use the first two coordinates when doing the draw operation. For reference, my coordinate systems approach is here. I round-trip everything through earth-centered earth-fixed. And there is an example going from lon/lat/alt to GOES fixed grid with the GRS80 globe at |
That is exactly what I was doing with Openlayers as an example where I did the transformation from lon/lat to whatever projection before I did the plotting. This method worked quite fine this way. I am not sure yet how this could be done in Metpy or Cartopy. I will have to look deeper into it to find out if it is possible. |
metpy/plots/mapping.py
Outdated
def make_latlon(attrs_dict, globe): | ||
# TODO: 180 here needs to be determined by longitude? | ||
# return ccrs.PlateCarree(globe=globe, central_longitude=180) | ||
# return ccrs.Geodetic(globe=globe) |
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.
E115 expected an indented block (comment)
I need to correct something I said above:
Height above the plane of projection is wrong. It turns out that proj.4 uses the altitude from the lon/lat/alt tuple as the z coordinates in the map projection, at least when using proj4.transform. Using my coordinate systems library, which is a thin wrapper on proj.4, I get the following:
The relevant transform call in proj.4 is https://github.com/jswhit/pyproj/blob/master/src/pj_transform.c#L83 See also the note about vertical units at http://proj4.org/parameters.html#vertical-units |
A few thoughts on these accessors from my perspective as an xarray maintainer:
|
@shoyer Thanks for the feedback. Using a single namespace |
Sure, that makes sense, but you can probably give them the same name? e.g., |
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.
So are we ready to fixup the little bot nits and get things green on this? An initial look over didn't have anything that jumped out.
97ebf99
to
9ccc199
Compare
examples/xarray_projections.py
Outdated
import matplotlib.pyplot as plt | ||
import xarray as xr | ||
|
||
import metpy.io # To ensure xarray accessors are available |
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.
F401 'metpy.io' imported but unused
9ccc199
to
5260534
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 think I'm good with this!
@jrleeman Are we ok picking up a hard dependency on xarray? |
Actually, I think I can work around that. |
I'm actually fine with the dependency since we're committing on the data model. |
d0fcc58
to
f97ddf5
Compare
694e39e
to
ee58f3a
Compare
Not sure what Circle's deal is, but everything else looks good here. I think this is ready @jrleeman . |
Well...just looked at coverage and it seems there's a test line that wasn't getting hit. Which meant there was a problem in the codecov config. |
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.
All happy now or still want to play with codecov?
18d7520
to
3d696e2
Compare
metpy/tests/test_cbook.py
Outdated
"""Test that the registry properly registers functions.""" | ||
reg = Registry() | ||
|
||
callback = lambda x: None |
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.
E731 do not assign a lambda expression, use a def
This adds a class with a register decorator that can be used to register callables under a string.
Default behavior of Python 2.7 gets confused by our having our own xarray module and trying to import the main library. Some future imports solve this...
Right now metpy.calc won't import without having pyproj installed, which is a bit more of a pain in the but than I want for a hard dependency right now.
Looks like we were missing some test lines
3d696e2
to
d9d7f00
Compare
I expect this to pass now. Assuming it does, go ahead and merge. |
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
Here's a first cut at automatically decoding CF projection information using an XArray accessor and making it available conveniently as a CartoPy projection. There are no docs or tests yet, but there is an example using our NARR test data set.
Open questions:
To save everyone the trouble of downloading to try it out (though please do!), here's what the example looks like:
And the result:
I've tested this locally with quite a few data sets. Everything coming from GOES-16 (both NOAAPORT and GRB) works fine. Many GRIB2 datasets work fine, with the exception of WW3 and MRMS; those failures are not related to the projection stuff. NARR, HRRR, NAM Alaska, and RTMA Guam are working fine.