issue_comments

I will try to figure out the reason for each of these test failures, but I'd appreciate help.

Would #3643 be the best place to offer contributions at this point, or somewhere else?

Re the 2nd 'failure mode': IIUC the current tests should still pass, and that's a separate (and correct) case; is that right?

That's right. The original test used test_reduce_keep_attrs as a template. Now, instead of defining one Variable object and passing it to both test conditions, a new Variable is defined and coarsened to check each setting of the global option.

I will merge master in and add similar tests for Rolling.

The recent changes I made to both the code and the tests result in a pass condition for both the dataset and variable version of test_coarsen_keep_attrs. I think adding the keep_attrs keyword argument to the Variable.coarsen method would be a breaking change, so I didn’t do that. Instead I added a check for the global setting inside the hidden method that reduces input Variable objects.

Note that there are at least two failure modes remaining:

• In the Dataset case, if the keep_attrs keyword is provided via the wrapper function (in this case mean) it results in the same TypeError as before because the converted functions don't expect the new keyword. I left the failing part of the test commented out because I'm not sure if this is supposed to be a supported usage mode.
• In the Variable case, the default behavior of dropping the attributes is permanent, so the operation is non-commutative if coarsening the same Variable instance (even when creating new objects). This seemed acceptable behavior for manipulating primitives.

Thank you for the review, I’ll incorporate those changes. No, the dataset test still raises xarray/core/duck_array_ops.py:311: TypeError: nanmean() got an unexpected keyword argument 'keep_attrs' and the variable test still fails due to assertion error. I think I understand why now though. Will test implementation and report.

Sorry, I changed this from a draft to ready for review accidentally, and it doesn't look like there is a way to reverse that operation.

The tests still fail after this latest change, but I think we are converging on a fix. The variable.coarsen method is applied to each of the arrays inside wrapped_func, so once that is fixed to recognize the global setting both tests should pass.

Your current Rolling implementation looks good. Does that work?

I couldn't find a test that checks attribute persistence across rolling window operations, but when I tested it interactively e.g.

dat = ds.rolling({'coord':1}, keep_attrs=True).mean() dat = ds.rolling({'coord':1}).mean(keep_attrs=True) assert dat.attrs == _attrs

it still doesn't seem to work. I agree that it is better to focus on Coarsen only for now.

For Coarsen, adding keep_attrs to the constructor is right, and then it needs to be assigned it to the object

Yes, I seem not to have included the assignment, or it got lost along the way. I added it with a new commit. Thank you for noticing that.

Unlike the Rolling implementation, you can add the logic for keeping attrs within the wrapped_func by branching on the value of self.keep_attrs; self there is the Coarsen object.

I think I see now. DatasetCoarsen._reduce_method.wrapped_func() accepts the DatasetCoarsen instance created by the xr.Dataset.coarsen() call. I will try to add a conditional inside that method's function to propagate the attributes along with the rest of the reduced object based on the value of the keep_attrs keyword.

It appears to be the mean function operating on the coarsen object xarray/tests/test_dataset.py#l.5676. The Rolling class uses _reduce_method() to check if the calling function is present in duck_array_ops, presumably to enable extensibility in line with NEP22.

FWIW the dataset object returned by that call in the test looks like this: values = array([[10. , 10.05050505, 10.1010101 , 10.15151515, 10.2020202 ], [10.25252525, 10.3030303 , 10.3535353...96 , 14.64646465, 14.6969697 , 14.74747475], [14.7979798 , 14.84848485, 14.8989899 , 14.94949495, 15. ]]) axis = (1,), skipna = None, kwargs = {'keep_attrs': True}, func = <function nanmean at 0x7f044d715268>, nanname = 'nanmean' ```

DatasetRolling.reduce passes along **kwargs. DatasetRolling.construct does not. Maybe it needs to?

I added code to pass the keyword through and handle it in the constructor of the Coarsen classes, and also Rolling because that seemed to be necessary as well. Now the test_dataset version of test_coarsen_keep_attrs fails due to a TypeError in the function nanmean(). The test_variable version fails the same way as before. A run of the whole test_dataset suite shows all the rest are running as they are on master.

This function nanmean() is a child of duck_array_ops._create_nan_agg_method() which seems like it should be happily passing along an arbitrary list of kwargs. The statement in the docstring

None of these functions should accept or return xarray objects.

makes me suspicious that this keep_attrs keyword shouldn't be reaching this section of the code though. Any guidance would be appreciated.