.where() and .fillna() should preserve attributes #1011
Conversation
| # preserve attributes | ||
| out = outobj._where(outcond) | ||
| out.attrs = self.attrs | ||
| if isinstance(out, Dataset): |
There was a problem hiding this comment.
Can you implement a private method defined on both Dataset/DataArray, something like _copy_attrs_from(self, other)? Then we can call out._copy_attrs_from(self) for a generic solution.
There was a problem hiding this comment.
Yes, good idea. Should the Dataset version check for the compatibility of other, or should we just assume that other will always have the same variables?
There was a problem hiding this comment.
I think it's fine to assume compatibility, given that this is only being called by internal methods. If other is not a Dataset then something has gone seriously wrong.
On the other hand, I think it might work to call DataArray.where with a Dataset as the argument -- worth double checking.
There was a problem hiding this comment.
I just tried actual = ds.a.where(ds > 1)and _where() returns a NotImplemented string
There was a problem hiding this comment.
OK, I think we're safe then, though we probably shouldn't be returning NotImplemented singletons to users :).
| out.attrs = self.attrs | ||
| if isinstance(out, Dataset): | ||
| for v in out: | ||
| out[v].attrs = self[v].attrs |
There was a problem hiding this comment.
Use out.variables[v].attrs = self.variables[v].attrs instead. Creating DataArray objects is pretty expensive compared to just pulling out existing Variable objects.
| @@ -1001,7 +1001,9 @@ def fillna(self, value): | |||
| if utils.is_dict_like(value): | |||
| raise TypeError('cannot provide fill value as a dictionary with ' | |||
| 'fillna on a DataArray') | |||
| return self._fillna(value) | |||
| out = self._fillna(value) | |||
There was a problem hiding this comment.
probably can put this in _fillna instead.
| ds.attrs['attr'] = 'ds' | ||
| ds.a.attrs['attr'] = 'da' | ||
| actual = ds.groupby('b').fillna(Dataset({'a': ('b', [0, 2])})) | ||
| self.assertTrue('attr' in actual.attrs) |
There was a problem hiding this comment.
use something like self.assertEquals(actual.attrs, {'attr': 'ds'}) instead
| actual = ds.groupby('b').fillna(Dataset({'a': ('b', [0, 2])})) | ||
| self.assertTrue('attr' in actual.attrs) | ||
| self.assertTrue(actual.attrs['attr'] == 'ds') | ||
| self.assertTrue(actual.a.name == 'a') |
fmaussion
force-pushed
the
keepattrs
branch
from
5bd5726 to
036c2f1
Compare
|
I implemented your comments with the exception of:
Do you mean this should/could be done in |
fmaussion
force-pushed
the
keepattrs
branch
from
635541a to
acc5541
Compare
fmaussion
force-pushed
the
keepattrs
branch
from
acc5541 to
9359092
Compare
I forgot that we hack this into |
|
OK. Sorry for the mess with Travis, I had a lot of typos :( It's getting late here and the new review system is a bit confusing |
| ds.a.attrs['attr'] = 'da' | ||
| actual = ds.groupby('b').fillna(Dataset({'a': ('b', [0, 2])})) | ||
| self.assertEqual(actual.attrs, ds.attrs) | ||
| self.assertTrue(actual.a.name == 'a') |
There was a problem hiding this comment.
still would be a good idea to use assertEqual here :)
There was a problem hiding this comment.
this is what happens when one does too many things at the same time without thinking :-(
Should be ok now, hopefully...
|
Thanks! |
Fixes #1009
There is probably a more elegant way to do it, but the internals of xarray still confuse me. I'm happy to learn!