Add keep_attrs to reduction methods #141
Conversation
| # Test dropped attrs | ||
| vm = self.va.mean() | ||
| self.assertEqual(len(vm.attrs), 0) | ||
| self.assertTrue(utils.dict_equal(vm.attrs, OrderedDict())) |
There was a problem hiding this comment.
I would just use assertEqual here, which works perfectly fine for dictionaries (you don't need utils.dict_equal unless you have numpy arrays instead your dicts).
| @@ -1027,6 +1031,11 @@ def reduce(self, func, dimension=None, **kwargs): | |||
| of summarized data and the indicated dimension(s) removed. | |||
| """ | |||
|
|
|||
| if keep_attrs: | |||
There was a problem hiding this comment.
Probably better to move it a closer to where attrs is used (at the end of the function).
If you like, you could make it a one-liner:
attrs = self.attrs if keep_attrs else {}(The attributes dictionary is always copied, so it doesn't matter whether you provide it as an OrderedDict or a normal dict.)
| @@ -20,6 +20,7 @@ | |||
| 'var2': ['dim1', 'dim2'], | |||
| 'var3': ['dim3', 'dim1'], | |||
| } | |||
| _attrs = {'attr1': 'value1', 'attr2': 2929} | |||
There was a problem hiding this comment.
This is another style issue (and you can see the existing test code isn't great at this), but in general I would rather not make global variables. It's more self-contained and obvious. In this case, you could simply move _attrs to inside test_reduce_keep_attrs.
There was a problem hiding this comment.
Yeah, no problem. I'm generally a fan of keeping fixtures as local as possible. In this case, I think I was matching the existing code style.
|
Could you update the signature and documentation of methods like |
|
The to update the signature of the reduce methods, you would need to update the function inside the |
|
Ok, is this what you mean? Sorry - wasn't tracking with you there. |
|
Yes, thanks! |
Add keep_attrs to reduction methods
fixes #138
This is a much cleaner version of #139.