issue_comments

Thanks a lot @amcnicho ! Glad to have you as a contributor!

Looks great! Any feedback from others? Otherwise will merge later today

Thanks @amcnicho !

Hello @amcnicho! 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! :beers:

Comment last updated at 2020-03-02 15:31:30 UTC

This looks excellent!

Could we add the change to whatsnew.rst?

Then I think we're ready to merge!

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.

That's looking good re coarsen! Do tests pass?

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.

@amcnicho

• Your current Rolling implementation looks good. Does that work?
• For Coarsen, adding keep_attrs to the constructor is right, and then it needs to be assigned it to the object
• Unlike the Rolling implementation, you can add the logic for keeping attrs within the wrapped_func (i.e. here ), by branching on the value of self.keep_attrs; self there is the Coarsen object.

Does that make sense?

Right, yes, this does get a bit nested... Appreciate you working through it.

At first glance, _reduce_method works differently in Rolling & Coarsen. You can consider leaving Rolling and just focusing on Coarsen.

~I think you want to catch the kwarg here~ and use it to decide whether to include the attrs here. It should not be a kwarg into wrapped_func.

Does that make sense?

...but then potentially it does conflict with the existing use of _reduce_method. I can have more of a look tonight.

CC @fujiisoup if you're more familiar with this code. And @dcherian has done some work on it.

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' ```

(sorry, I didn't see your message before my message prior)

Do you know what's calling nanmean with keep_attrs? I think should be handling keep_attrs before we get to those numerical functions on variables.

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

Recent commits look good @amcnicho !

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.

Thanks for the PR and tests @amcnicho . Are you up for adding the code to pass through keep_attrs?