-
-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add invariant check for IndexVariable.name #8906
base: main
Are you sure you want to change the base?
Conversation
Thanks @dcherian, that's a good check to have I think. Most (if not all?) errors seem to be multi-index level variables that wrongly have their dimension name set as IndexVariable |
@@ -2590,12 +2590,14 @@ class IndexVariable(Variable): | |||
unless another name is given. | |||
""" | |||
|
|||
__slots__ = () | |||
__slots__ = ("_name",) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we remove the name
property instead or is that much much harder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would be better indeed but this is likely more work (not sure how much, though).
IndexVariable still needs a deeper refactor (#8124), or even be eventually dropped?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the .name
property used?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After a quick check: internally only in a few places (conventions / multiindex check, dataarray creation, maybe groupby?) actually.
Externally I have no idea (IndexVariable is public API in theory).
Here's a wilder idea: @property.getter
def name(self) -> Hashable:
name, = self._dims
return name
@property.setter
def name(self):
# maybe warn for a while first
raise NotImplementedError |
2083caf
to
ad56e7b
Compare
@benbovy this seems to be the root cause of #8646, the variable name in
Dataset._variables
does not matchIndexVariable.name
.A good number of tests seem to fail though, so not sure if this is a good chck.
whats-new.rst
api.rst