Systematic Coordinate Identification #871

jthielen · 2018-06-08T23:16:31Z

A first cut at implementing #860, this implements systematic coordinate mapping utilities to the Dataset accessor according to CF standards/recommendations/options and uses those to assign appropriate axis attributes to the coordinates in parse_cf. This also adds some properties to the DataArray accessor to obtain easy access to the coordinates corresponding to a particular axis.

This PR is a work is progress, but I wanted to get it out there to run it by the checks, and to get some initial feedback. Once the initial implementation of everything (roughly) looks good, I plan on working on tests.

(Also, @dopplershift and @jrleeman, for now, I decided to go with vertical instead of level or levels, and time instead of times, as those seemed more natural as I was working through it.)

ahuang11 · 2018-06-12T04:53:40Z

metpy/xarray.py

+    @staticmethod
+    def _check_vertical(var):
+        # Check for standard name (CF option)
+        if var.attrs.get('standard_name') in {'air_pressure', 'height', 'geopotential_height',


level is pretty common too I think

Is that part of CF?

I wasn't able to find level in the standard name table, but I did find model_level_number, which could be good to add.

Also, there's an appendix in the CF conventions with a number of additional vertical coordinate standard names...should I include those as well?

ahuang11 · 2018-06-12T04:58:23Z

metpy/xarray.py

+    def _fixup_coordinate_map(coord_map, var):
+        """Ensure sure we have coordinate variables in map, not coordinate names."""
+        for axis in coord_map:
+            if type(coord_map[axis]) == str:


I think it's more standard to use if isinstance(coord_map[axis], str):

jthielen · 2018-06-13T20:49:50Z

One thing I've been thinking about as I work through applications of this...right now, using this (for example, to iterate over levels) would look something like

temperature = data.parse_cf('Temperature_isobaric')
for level in temperature.metpy.vertical:
    temperature_at_level = temperature[temperature.metpy.vertical == level]

This could get verbose rather quickly with lots of calls to the coordinate variables. Would it be worth replacing and/or supplementing it with something like this?

temperature = data.parse_cf('Temperature_isobaric')
x, y, vertical = rh.coordinate_map('x', 'y', 'vertical')
for level in vertical:
    temperature_at_level = temperature[vertical == level]

(this should be fairly straightforward to implement as a wrapper for what I now have as MetPyAccessor._axis)

jrleeman · 2018-06-14T14:00:20Z

I'm in favor of that - maybe spell it as just coordinates though? Even less typing 😄

jthielen · 2018-06-14T14:07:25Z

@jrleeman I've added it in addition to the properties for now, but let me know if it is better as a replacement.

jrleeman

A few minor things - a more in-depth review is next

jrleeman · 2018-06-14T14:05:28Z

metpy/xarray.py

 @xr.register_dataarray_accessor('metpy')
 class MetPyAccessor(object):
    """Provide custom attributes and methods on XArray DataArray for MetPy functionality."""

+    nice_to_cf_axis_map = {'time': 'T', 'vertical': 'Z', 'y': 'Y', 'x': 'X'}


A nit, but I'd probably call this something like readable_to_cf_names or similar. I've started trying to avoid map as part of variable names as it sometimes is redundant or even confusing in some contexts.

jrleeman · 2018-06-14T14:06:25Z

metpy/xarray.py

+            for coord_var in self._data_array.coords.values():
+                if coord_var.attrs.get('axis') == self.nice_to_cf_axis_map[axis]:
+                    return coord_var
+            raise AttributeError(axis + ' attribute is not available due to lack of ' +


Could probably stop after not available.

jrleeman · 2018-06-14T14:08:48Z

metpy/xarray.py

+    @staticmethod
+    def _check_vertical(var):
+        # Check for standard name (CF option)
+        if var.attrs.get('standard_name') in {'air_pressure', 'height', 'geopotential_height',


Is that part of CF?

jthielen · 2018-06-15T21:08:36Z

@jrleeman Some notes about the changes I've made based on our discussions:

The suggested name change from coord_map to coordinate_names didn't quite seem right as I got to implementing it, since you could be passing in a dict of coordinate names or coordinate variables, so for now, I made it coordinates.
I first tried just generalizing _check_x and _check_y, but I noticed many overlaps with the others once I made that change, so I just went ahead and combined all of them. Let me know what you think of the approach now.
Fixing the issue I was having parsing a full dataset (crs coordinates not matching up between variables) was actually as easy as implementing __eq__ and __ne__ on the CFProjection object, so hopefully that should be working now.

jrleeman

I'm happy after this refactor - I think we're in good shape now, but let's get @dopplershift to weigh in today and then test time!

jrleeman · 2018-07-02T16:33:51Z

Unless @dopplershift has issues, I'd say lets get this in ASAP.

dopplershift

In general, I'm ready for this to go in. Just a couple minor things. This xarray tutorial will be an especially nice addition.

dopplershift · 2018-07-02T16:48:45Z

metpy/tests/test_xarray.py

+def test_bad_coordinate_type(test_var):
+    """Test that an AttributeError is raised when a bad axis/coordinate type is given."""
+    with pytest.raises(AttributeError) as exc:
+        axis, = test_var.metpy.coordinates('bad_axis_type')


You can just call the function here, no need to do anything with the return value, so just:

test_var.metpy.coordinates('bad_axis_type')

I had tried that, but because the coordinates method is a generator function, I needed to assign it in order for the underlying function to run and raise the error. Is there a better way around this?

Ah, generator. You could wrap the call in next(...).

dopplershift · 2018-07-02T16:51:49Z

tutorials/xarray_tutorial.py

+#
+# Additionally, we take a few optional steps to clean up our dataset for later use:
+#
+# - Drop the extra grid mapping variable since we have already parsed that information


I'm not clear on why we're going to the effort of dropping a variable. Does it help with printing or something?

Good point...I was being over-zealous in my clean up of the Dataset.

Adds coordinate mapping utilities to the Dataset accessor, and uses them in `parse_cf` to relabel the `axis` attributes appropriately. Then, adds in methods to the DataArray accessor so that the spatiotemporal coordinates can be easily accessed.

This commit adds a new tutorial to demonstrate the extent of MetPy's xarray integration, and how to perform basic operations. This tutorial should be kept up to date as integration with xarray improves.

This commit adds tests for systematic coordinate identification.

dopplershift

Pending CI.

jthielen · 2018-07-02T22:12:12Z

Just after this was merged, I discovered a cleaner way of checking the equality of DataArray's: http://xarray.pydata.org/en/stable/combining.html#equals-and-identical. Should I submit a follow-up PR cleaning up the tests, or is what is there now good enough?

jrleeman · 2018-07-03T16:10:57Z

If it's not terribly time-consuming I'd say a follow-on PR would be good.

jthielen added Area: Infrastructure Pertains to project infrastructure (e.g. CI, linting) Type: Feature New functionality labels Jun 8, 2018

jthielen added this to the 0.9 milestone Jun 8, 2018

jthielen requested review from dopplershift and jrleeman June 8, 2018 23:16

ahuang11 reviewed Jun 12, 2018

View reviewed changes

jrleeman reviewed Jun 14, 2018

View reviewed changes

jrleeman reviewed Jun 18, 2018

View reviewed changes

jthielen mentioned this pull request Jun 22, 2018

Cross-Section Normal Component #652

Closed

dopplershift requested changes Jul 2, 2018

View reviewed changes

jthielen added 2 commits July 2, 2018 10:57

Add xarray tutorial

475673b

This commit adds a new tutorial to demonstrate the extent of MetPy's xarray integration, and how to perform basic operations. This tutorial should be kept up to date as integration with xarray improves.

jthielen changed the title ~~WIP: Systematic Coordinate Identification~~ Systematic Coordinate Identification Jul 2, 2018

Add tests

a74670d

This commit adds tests for systematic coordinate identification.

dopplershift approved these changes Jul 2, 2018

View reviewed changes

dopplershift merged commit b848a06 into Unidata:master Jul 2, 2018

jthielen mentioned this pull request Jul 3, 2018

xarray Coordinate Identification Cleanup #880

Merged

jthielen mentioned this pull request Nov 12, 2018

parse_cf does not work when multiple grid mappings are present #965

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Systematic Coordinate Identification #871

Systematic Coordinate Identification #871

jthielen commented Jun 8, 2018

ahuang11 Jun 12, 2018

jrleeman Jun 14, 2018

jthielen Jun 14, 2018 •

edited

Loading

ahuang11 Jun 12, 2018

jthielen commented Jun 13, 2018

jrleeman commented Jun 14, 2018

jthielen commented Jun 14, 2018

jrleeman left a comment

jrleeman Jun 14, 2018

jrleeman Jun 14, 2018

jrleeman Jun 14, 2018

jthielen commented Jun 15, 2018

jrleeman left a comment

jrleeman commented Jul 2, 2018

dopplershift left a comment

dopplershift Jul 2, 2018

jthielen Jul 2, 2018

dopplershift Jul 2, 2018

dopplershift Jul 2, 2018

jthielen Jul 2, 2018

dopplershift left a comment

jthielen commented Jul 2, 2018

jrleeman commented Jul 3, 2018

Systematic Coordinate Identification #871

Systematic Coordinate Identification #871

Conversation

jthielen commented Jun 8, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jthielen Jun 14, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jthielen commented Jun 13, 2018

jrleeman commented Jun 14, 2018

jthielen commented Jun 14, 2018

jrleeman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jthielen commented Jun 15, 2018

jrleeman left a comment

Choose a reason for hiding this comment

jrleeman commented Jul 2, 2018

dopplershift left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dopplershift left a comment

Choose a reason for hiding this comment

jthielen commented Jul 2, 2018

jrleeman commented Jul 3, 2018

jthielen Jun 14, 2018 •

edited

Loading