issue_comments

~Does anyone know why the xr.open_dataset(....) call is echoed in the warning message. Is this intentional? Cc @dcherian @DWesl~

It seems you've already figured this out, but for anyone else with this question, the repeat of the call on that file is part of the warning that the file does not have all the variables the attributes refer to. You can fix this by recreating the file with the listed variables added (areacella, or by deleting the attribute from the variables (cell_measures). You can also ignore the warning using the machinery in the warnings module.

I think this looks good.

Looks good to me. I was wondering where those docstrings were.

That's two people with that view so I made the change.

Again, I feel that the quality flags are essentially meaningless on their own, useful primarily in the context of their associated variables, like the items currently put in the XArray coords attribute, which, admittedly, is only those variables identified by CF as dimension or auxiliary coordinates at the moment, and should remain associated with the relevant variable even if it is extracted into a DataArray. Since all of the other people who have opinions on the matter seem to disagree with me, I changed the code to preserve the present behavior with regards to ancillary_variables. I can always monkey-patch it back in if it really bothers me, or add a Dataset.__getitem__ wrapper to xarray-contrib/cf-xarray so that the ancillary_variables stay associated when I pull variables out, or move back to SciTools/iris.

On a related note, I should probably check whether this breaks conversion to an iris.Cube.

You are correct; ancillary_variables is neither grid_mapping or bounds.

My personal view is that the quality information should stay with the variable it describes unless explicitly dropped; I think your view is that quality information can always be extracted from the original dataset, and that no variable should carry quality information for a different variable. At this point it would be simple to remove ancillary_variables from the attributes processed by this PR. There was a suggestion earlier of adding a decode_aux_vars argument to control the new behavior as a means of avoiding back-compatibility breaks like this one. I will leave that as a question for the maintainers; there is also some related discussion at #4215.

I should point out that a similar situation arises for grid_mapping; ds.coords["time"] will include the grid_mapping variable in its coordinates. In contrast, ds.coords["x"] will not include the bounds for the x variable, since it has more dimensions than ds.coords["x"]

The MinimumVersionsPolicy error appears to be a series of internal conda errors, and is probably unrelated.

pydata/xarray-data#19

The rasm dataset has coordinates xc and yc, which reference bounds xv and yv respectively, which I do not see in the variable list with decode_coords=False. It would appear that pydata/xarray-data#4 did not include the bounds in the updated dataset when adding coordinates to rasm.nc, so this warning is correct. I do not know that file, so I'm probably not the best person to add bounds. Should I wait for an update to pydata/xarray-data, or should I ask sphinx to ignore the warning?

Another option is to just delete the bounds attributes of xc and yc in rasm.nc

The example the doc build doesn't like: ```python ds = xr.tutorial.load_dataset("rasm") ds.to_zarr("rasm.zarr", mode="w") import zarr

zgroup = zarr.open("rasm.zarr") print(zgroup.tree()) dict(zgroup["Tair"].attrs) `` I'll need to look into therasm` dataset to figure out why there is a warning now.

On 5/31/2019 11:50 AM, dcherian wrote:

It isn't just MetPy though. I'm sure there's existing code relying on adding |grid_mapping| and |bounds| to |attrs| in order to write CF-compliant files. So there's a (potentially big) backward compatibility issue. This becomes worse if in the future we keep interpreting more CF attributes and moving them to |encoding| :/.

At present, the proper, CF-compliant way to do this is to have both |grid_mapping| and |bounds| variables in |data_vars|, and maintain the attributes yourself, including making sure the variables get copied into the result after relevant |ds[var_name]| and |ds.sel(axis=bounds)| operations.

If you decide to move these variables to |coords|, the |bounds| variables will still get dropped on any subsetting operation, including those where the relevant axis was retained, the |grid_mapping| variables will be included in the result of all subsetting operations (including pulling out, for example, a time coordinate), and both will be included in some |coordinates| attribute when written to disk, breaking CF compliance.

This PR only really addresses getting these variables in |coords| initially and keeping them out of the global |coordinates| attribute when writing to disk.

Since I'm doing this primarily to get grid_mapping and bounds
variables out of ds.data_vars.

I'm +1 on this but I wonder whether saving them in |attrs| and using that information when encoding coordinates would be the more pragmatic choice.

You have a point about |grid_mapping|, but applying the MetPy approach of saving the information in another, more directly useful format (|cartopy.Projection| instances) immediately after loading the file would be a way around that.

For |bounds|, I think |pd.PeriodIndex| would be the most natural representation for time, and |pd.IntervalIndex| for most other 1-D cases, but that still leaves |bounds| for two-or-more-dimensional coordinates.

That's a design choice I'll leave to the maintainers.

We could define |encoding| as containing a specified set of CF attributes that control on-disk representation such as |units|, |scale_factor|, |contiguous| etc. and leaving everything else in |attrs|. A full list of attributes that belong in |encoding| could be in the docs so that downstream packages can fully depend on this behaviour.

Currently I see |coordinates| is interpreted and moved to |encoding|. In the above proposal, this would be left in |attrs| but its value would still be interpreted if |decode_coords=True|.

What do you think?

At present, |set(ds[var_name].attrs["coordinates"].split())| and |set(ds[var_name].coords) - set(ds[var_name].indexes[dim_name])| would be identical, since the |coordinates| attribute is essentially computed from the second expression on write.

Do you have a use case in mind where you need specifically the list of CF auxiliary coordinates, or is that just an example of something that would change under the new proposal? I assume |units| would be moved to |encoding| only for |datetime64[ns]| and |timedelta64[ns]| variables.

This PR currently puts grid_mapping and bounds in encoding once it is done with them. Is that where XArray wants to put them, or should they be somewhere else?

Should I change this to put grid_mapping and bounds back in attrs, or should I leave them in encoding?

I think the choice is between attrs and encoding, not both.

If it helps lean your decision one way or the other, attrs tends to stay associated with Datasets through more operations than encoding, so parse_cf() would have to be called fairly soon after opening if the information ends up in encoding, while putting it in attrs gives users a bit more time for that.

The test failures seem to all be due to recent changes in cftime/CFTimeIndex, which I haven't touched.

Is sticking the grid_mapping and bounds attributes in encoding good, or should I put them back in attrs?

I just noticed pandas.PeriodIndex would be an alternative to pandas.IntervalIndex for time data if which side the interval is closed on is largely irrelevant for such data.

Is there an interest in using these for 1D coordinates with bounds? I think ds.groupby_bins() already returns an IntervalIndex.

Switched to use in rather than is not None.

Re: grid_mapping in .encoding not .attrs MetPy assumes grid_mapping will be in .attrs. Since the xarray documentation mentions this capability, should I be making concurrent changes to MetPy to allow this to continue?

If so, would it be sufficient to change their .attrs references to .encoding and mentioning in both sets of documentation that the user should call ds.metpy.parse_cf() immediately after loading to ensure the information is available for MetPy to use? I don't entirely understand the accessor API.

This is briefly mentioned above, in https://github.com/pydata/xarray/pull/2844#discussion_r270595609 The rationale was that everywhere else xarray uses CF attributes for something, the original values of those attributes are recorded in var.encoding, not var.attrs, and consistency across a code base is a good thing. Since I'm doing this primarily to get grid_mapping and bounds variables out of ds.data_vars, I don't have strong opinions on the subject.

If you feel strongly to the contrary, there's an idea at the top of this thread for getting bounds information encoded in terms xarray already uses in some cases (Dataset.groupby_bins()), and the diffs for this PR should help you figure out what needs changing to support this.

For grid_mapping there's http://xarray.pydata.org/en/latest/weather-climate.html#cf-compliant-coordinate-variables which is enough for my uses.

I can shift this to use encoding only, but I'm having trouble figuring out where that code would go.

Would the preferred path be to create VariableCoder classes for each and add them to encode_cf_variable, then add tests to xarray.tests.test_coding?

Related to #1475 and #2288 , but this is just keeping the metadata consistent where already present, not extending the data model to include bounds, cells, or projections. I should add a test to ensure saving still works if the bounds are lost when pulling out variables.