Avoid internal use of deprecate attrs and meta properties
#3283
Conversation
This avoids warnings in a number of operations.
| params.attrs['begin'] = var.bins.constituents['begin'].copy() | ||
| params.attrs['end'] = var.bins.constituents['end'].copy() | ||
| params.coords['begin'] = var.bins.constituents['begin'].copy() | ||
| params.coords['end'] = var.bins.constituents['end'].copy() |
There was a problem hiding this comment.
Can these leak into the output? And should they?
There was a problem hiding this comment.
As far as I can tell params is not used directly?
| dim = edges.dims[-1] | ||
| if dim not in x.bins.meta: | ||
| if x.meta.is_edges(dim): | ||
| if dim not in x.bins.coords: |
There was a problem hiding this comment.
This is a breaking change that removes some support for attrs. Should we actually do this now?
There was a problem hiding this comment.
I thought in this case this was simply overlooked when changing attrs -> unaligned coords? We did other similar breaking changes (slicing, transform_coords) already, didn't we?
There was a problem hiding this comment.
But this code doesn't store attrs, it looks for edges in the input. So the user is in control of where they put their edges, not us. Or am I misunderstanding this code?
There was a problem hiding this comment.
No you did not misunderstand, but didn't we also change, e.g., transform_coords to not look for attrs?
There was a problem hiding this comment.
Yes, looks like it. But that was when we moved to aligned coords, not when we deprecated attrs. I'd actually say we should change it along with the code here to use deprecated_meta.
There was a problem hiding this comment.
But when should we make the breaking change then? Why not now, if the similar changes have already happened recently?
I would have thought we do this when removing attrs. The earlier changes may have been a mistake and we shouldn't use that to continue down the same path.
But maybe the whole point is moot because nobody uses attrs like this?
There was a problem hiding this comment.
This is unrelated to removal of attrs, isn't it? This is about the addition of the coord alignment flag. The "only" reason all those operations also considered attrs was their double-use for unaligned coords.
The earlier changes may have been a mistake
In what sense? Are you considering going back to attrs instead of unaligned coords?
There was a problem hiding this comment.
So do you see the introduction of unaligned coords also as removing that role from attrs?
There was a problem hiding this comment.
Yes, isn't this what we have done for other operations such as transform_coords?
This avoids warnings in a number of operations.