Don't set encoding attributes on bounds variables.

[dcherian]

Here's a proposed fix for #2436 and #2921. Ping @spencerkclark @mathause @klindsay28

1. Removes certain attributes from bounds variables on encode.
2. open_mfdataset: Sets encoding on variables based on encoding in first file.

• Closes to_netcdf with decoded time can create file with inconsistent time:units and time_bounds:units #2921
• Tests added
• Fully documented, including whats-new.rst for all changes and api.rst for new API

[pep8speaks]

Hello @dcherian! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2019-06-24 21:51:22 UTC

[mathause]

Thanks for putting this together.

I think this can work as long as time has an encoding - however, if it doesn't we might end up with different units for time and time_bounds again. I am not sure, actually, just something to check.

[dcherian]

Yeah, this still doesn't work as @klindsay28 just pointed out to me.

I still don't understanding how the units attribute/encoding is being treated. I'll get back to this soon.

[spencerkclark]

Thanks for taking this on @dcherian. I think this is on the right track. Do you have an explicit example of the edge case(s) you still have in mind?

[dcherian]

Thanks @spencerkclark, I'm going by the example in #2921. I've added that now as a test. I also test for the slight non-compliance case where time_bounds has different units already set.

@mathause would this test catch the issue you were encountering?

[mathause]

Great - this does indeed solve my problem (read multiple files with open_mfdataset and write as one). However, it still 'fails' - is not cf-compiant to be more precise - for a new dataset created in xarray, see:

import xarray as xr
import pandas as pd
time = pd.date_range('2000-01-16', periods=1)
time_bounds = pd.date_range('2000-01-01', periods=2, freq='MS')

ds = xr.Dataset(dict(time=time, time_bounds=time_bounds))
ds.time.attrs['bounds'] = 'time_bounds'

xr.conventions.cf_encoder(ds.variables, ds.attrs)

(compare the units). This might be difficult to solve, as xarray currently assumes all variables can be encoded independently.

[spencerkclark]

This might be difficult to solve, as xarray currently assumes all variables can be encoded independently.

I was hoping someone wouldn't bring that case up :). But I agree I think it's something we should discuss.

I'm sort of torn on whether it is worth the complexity. Part of me feels like if a user is concerned/clever enough to explicitly create such linking attributes (e.g. 'bounds' pointing to 'time_bounds') in their Datasets that we could leave it as their responsibility to make sure the 'units' and 'calendar' encodings match for each of the variables as well. For example, while a bit verbose, this would work as desired:

import xarray as xr
import pandas as pd
time = pd.date_range('2000-01-16', periods=1)
time_bounds = pd.date_range('2000-01-01', periods=2, freq='MS')

ds = xr.Dataset(dict(time=time, time_bounds=time_bounds))
ds.time.encoding['bounds'] = 'time_bounds'
ds.time.encoding['units'] = 'days since 2000-01-01'
ds.time.encoding['calendar'] = 'proleptic_gregorian'

ds.time_bounds.encoding['units'] = ds.time.encoding['units']
ds.time_bounds.encoding['calendar'] = ds.time.encoding['calendar']

That said, that case is something that in principle we could address, and maybe it is worth thinking about if we ever consider increasing the functionality related to cell bounds coordinates (e.g. #1475).

[mathause]

Could we issue a warning here?

else:
    warning.warn("The attribute 'units' is not the same in the variable "
"'time' and it's associated bounds 'time_bnds',"
" which is not cf-compliant.")

or some such

[dcherian]

Do we need to? xarray allows writing CF-non-compliant files anyway...

[spencerkclark]

I think the warning you added is the perfect approach. It will still be issued in the case of @mathause's example, but will still allow a user to write a non-CF-compliant file without a warning if the encoding attributes do not need to be computed on the fly.

[mathause]

Yes, I agree - I don't intend to actually create such a dataset by hand anytime soon ;) but it would be super hard to track this issue down - can we issue a warning instead? See my inline comment.

[dcherian]

This case might actually be really easy to fix since we already have xr.conventions._update_bounds_attributes() from Fabien's previous PR (see below).

I think we would just need to call this prior to calling encode_cf_variable() in

xarray/xarray/conventions.py

Line 622 in 612d390

new_vars = OrderedDict((k, encode_cf_variable(v, name=k))

def _update_bounds_attributes(variables):
    """Adds time attributes to time bounds variables.

    Variables handling time bounds ("Cell boundaries" in the CF
    conventions) do not necessarily carry the necessary attributes to be
    decoded. This copies the attributes from the time variable to the
    associated boundaries.

    See Also:

    http://cfconventions.org/Data/cf-conventions/cf-conventions-1.7/
         cf-conventions.html#cell-boundaries

    https://github.com/pydata/xarray/issues/2565
    """


    # For all time variables with bounds
    for v in variables.values():
        attrs = v.attrs
        has_date_units = 'units' in attrs and 'since' in attrs['units']
        if has_date_units and 'bounds' in attrs:
            if attrs['bounds'] in variables:
                bounds_attrs = variables[attrs['bounds']].attrs
                bounds_attrs.setdefault('units', attrs['units'])
                if 'calendar' in attrs:
                    bounds_attrs.setdefault('calendar', attrs['calendar'])

[spencerkclark]

I think we would just need to call this prior to calling encode_cf_variable()

I'm afraid it's not quite that simple :).

In cases where someone creates a datetime-like variable in memory (e.g. with pd.date_range in @mathause's example), unless they explicitly add encoding attributes, 'units' and 'calendar' will need to be computed on the fly. This happens inside of encode_cf_variable, currently on a per-variable basis, so trying to make sure the attributes are equal before that step will not help, because they might not exist.

[dcherian]

Thanks @spencerkclark & @mathause. I now understand the issue better. #2921 was confusing in that @klindsay28 was trying to write an encoded dataset.

I've updated the tests to use @mathuse's example. The code now updates the time_bounds variable with the encoding 'units' and 'calendar' of the time variable. It also throws a warning when encoding.units is not specified for variables with a bounds attribute.

Do you think this is a good approach?

[spencerkclark]

Thanks @dcherian -- @mathause's suggestion of adding a warning was helpful, and I think your addition to potentially propagate the 'units' and 'calendar' encoding parameters from the root to the bounds coordinate is also good. I just have a few more comments and a question.

[spencerkclark]

A general question -- would we consider 'bounds' to be an encoding parameter (like 'units' or 'calendar')? In other words should we expect it to be in the encoding dictionary or attrs dictionary at this stage? I feel like it may be more intuitive as part of encoding, but currently I know that we don't treat it that way when decoding files.

[dcherian]

In my mental model, encoding attributes are those that control on-disk representation of the data. bounds counts as an attr to my mind since it's an attribute that links the variable to another variable.

A definition or list of what goes in encoding and what goes in attrs would make a good addition to the docs.

[spencerkclark]

In my mental model, encoding attributes are those that control on-disk representation of the data.

I think this is fair; I guess I was going off of the mental model of encoding parameters defined as "attributes that are potentially required for decoding all the variables in a file," in which case 'bounds' could qualify. I think your definition is probably cleaner, because it requires that encoding parameters control how the variable they are attached to is represented on disk (as opposed to another variable).