-
Notifications
You must be signed in to change notification settings - Fork 53
Some issues with type annotations #143
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
Comments
Thanks for the great feedback @BvB93, and for collecting it here @asmeurer. I'll try and fix the straightforward ones now.
It's valid in Python 3.8 with
No, I don't think that should be changed in the spec, that would just make it harder to read. It's an implementation issue.
The return type isn't relevant, it must be an object with the minimum properties. So the suggestion of using a
No, this is on purpose. We cannot have valid types for library-specific classes/objects.
Same answer, yes in implementation, no in spec. |
Also an implementation issue. I'd simply type it as |
Already covered by EDIT: after reviewing, that actually makes it less clear. It should be treated just like other objects that are covered by |
I'm actually not sure I can be convinced that accepting |
The annotation wasn't |
Another issue I just noticed. The dunder methods ( |
Continuing from numpy/numpy#18585 (comment):
@asmeurer when used as parameter type, all unions involving
The So no, the most correct type for the more general ExamplesAn example of from typing import TypeVar, Protocol, Any
PyCapsule = Any # TODO
_T_contra = TypeVar("_T_contra", contravariant=True)
class SupportsDLPack(Protocol[_T_contra]):
def __dlpack__(self, /, *, stream: None | _T_contra = ...) -> PyCapsule: ... |
Two problems recently discovered in numpy/numpy#19969:
|
As all items in this tracking issue have been addressed, will close. Any further issues can be discussed in a new issue and resolved in follow-up PRs. |
Some issues with type annotations that were brought up by @BvB93 at numpy/numpy#18585
asarray
, it isn't clear how to defineNestedSequence
(typing
annotations don't support recursive types).asarray
needs to also includearray
as a valid type forobj
(array
should not be considred a subtype ofNestedSequence
)NestedSequence
type inasarray
)stack
andconcat
, the type forarrays
should beTuple[array, ...]
instead ofTuple[array]
stack
andconcat
(and possibly others),Tuple
is too restrictive vs.Sequence
.unique
can be implemented as an@overload
(see ENH: Implementation of the NEP 47 (adopting the array API standard) numpy/numpy#18585 (comment)). I don't know if this needs to be changed in the spec.finfo
asfinfo
doesn't make sense iffinfo
is a function (see ENH: Implementation of the NEP 47 (adopting the array API standard) numpy/numpy#18585 (comment)).__getitem__
and__setitem__
should include theellipsis
type (ENH: Implementation of the NEP 47 (adopting the array API standard) numpy/numpy#18585 (comment))(update:__len__
should returnint
__len__
is slated for removal from the spec; see Specify shape behavior #289)shape
is currentlyUnion[ Tuple[ int, …], <shape> ]
, which needs to be addressed (there is a TODO in the spec for this). (update: see Specify shape behavior #289)Some of the other types for properties also use<...>
types. We should make all the type annotations be valid Python/mypy that can be used in the annotations in the signature.Should the array object subclass fromtyping.Protocol
(ENH: Implementation of the NEP 47 (adopting the array API standard) numpy/numpy#18585 (comment))? (I don't know if this is relevant for the spec).__add__
, and so on) should includeint
,float
, (andbool
as appropriate) as allowed types forother
.The text was updated successfully, but these errors were encountered: