Annotations for .data_vars() and .coords() #3207
Conversation
| raise NotImplementedError() | ||
|
|
||
| @property | ||
| def dims(self) -> Union[Mapping[Hashable, int], Tuple[Hashable, ...]]: |
There was a problem hiding this comment.
Why the change here from return self._data.dims?
There was a problem hiding this comment.
Because DataArray.dims is a Tuple[Hashable, ...] whereas Dataset.dims is a Mapping[Hashable, int]
There was a problem hiding this comment.
See below for the two implementations, which are identical in code but different in signature.
The method needs stay in the abstract class because other methods of the abstract class invoke it; this signature with NotImplementedError() guarantees that the methods of the abstract class can only use dims as the common denominator of the two output types, which is a Collection[Hashable].
| self.update({key: value}) | ||
|
|
||
| @property | ||
| def indexes(self): | ||
| def _names(self) -> Set[Hashable]: |
There was a problem hiding this comment.
Is this new? Should this be on the abstract class?
There was a problem hiding this comment.
It's a property invoked by the methods of the abstract class, defined in both the implementations.
There was a problem hiding this comment.
Here, I'm not so sure - do all concrete classes need to define this? Generally we wouldn't define private methods on an abstract class (even if all currently concrete classes implement it), because the implementation would be up to the concrete class?
There was a problem hiding this comment.
What I basically did was turn AbstractCoordinates from a mixin (that is, a class that invokes own methods that are only defined in a subclass) into a C++-style abstract class.
mypy, justifiably, doesn't like mixins - short of hacking your way through with # type tags.
This, in synthesis, is how it was before:
class C:
def g(self) -> int:
return self.f()
class D(C):
def f(self) -> int:
return 123
mypy correctly states:
3: error: "C" has no attribute "f"
There was a problem hiding this comment.
Right - I agree this should either be an abstract class without implementations or a mixin with implementations - not a bit of each. It's a bit odd we call this class Abstract but then enforce a _names property.
That a nice upside of mypy - we get to find those things out
I agree we're in a reasonable local maximum here.
| self.update({key: value}) | ||
|
|
||
| @property | ||
| def indexes(self): | ||
| def _names(self) -> Set[Hashable]: |
* upstream/master: chunk sparse arrays (pydata#3202) Annotations for .data_vars() and .coords() (pydata#3207) Remove duck_array_ops.as_like_arrays() (pydata#3204) Match mypy version between CI and pre-commit hook (pydata#3203) BUG fix +test .sel method gives error with float32 values (pydata#3153) fix precommit file (pydata#3201) Enforce mypy compliance in CI (pydata#3197)
No description provided.