-
Notifications
You must be signed in to change notification settings - Fork 5
✨: HasX attributes #34
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
base: main
Are you sure you want to change the base?
Conversation
@jorenham @NeilGirdhar @lucascolley this doesn't resolve the current discussion re a |
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.
I like the Has*
protocols. Optype will cover the CanArray*
ones I think. There might be some that can't be expressed because Self
can't be passed as generic type argument, but I suppose we can deal with it when needed.
One nit is that ...
are not needed when there's a docstring, which already counts as an expression or statement or something.
What do we do about docstrings?
Yes, my pylint is complaining. But IMO empty methods should have |
Hmm good question. Maybe a numpy-esque |
Yea I guess there's something to be said for that. It just looks a bit weird to me to see a |
8096aff
to
3793c1f
Compare
cb3dbb3
to
dec1842
Compare
dec1842
to
53ddb98
Compare
53ddb98
to
a5e3d5b
Compare
@jorenham I added tests and made the protocols public. |
@jorenham This should cover all the array attributes. |
... | ||
|
||
|
||
class HasSize(Protocol): |
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.
What's the use-case of this one? Is there anything this can help with, that HasShape
can't?
Put differently; should we make this public API or not?
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.
Shouldn't we have Protocols for all attributes in the Array API?
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.
I don't see why we should write a protocol if there's no use for it.
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.
def is_sized(obj: Any, /) -> TypeGuard[HasSize]: ...
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.
ok, and what would you use that for in a real-world scenario
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.
IDK personally, but the Array API is pretty sparing.
I could see using something like:
def check_min_size(obj: object) -> bool:
if not is_sized(obj) or obj.size is None:
raise Exception
return obj.size > 10
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.
Hm yea I guess, but that's probably pretty niche, right?
I guess I just feel like we should first start with fixing the big typing problems that the array-api libs are facing, and I don't consider this to be one of those. That's why I opened #22, so that we can get a better understanding of what those problems actually are. And right now it just feels like we're trying to mimick the runtime API, and blindly hope that it will somehow be helpful, without knowing how exactly.
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.
Not disagreeing that there are other big issues to solve, but people also want and need the more straightforward static Array API. I've had many people ask me about doing straightforward things like
def square[T=Array](x: T) -> T: ...
So yeah, I do think there's a lot of value in starting to fill out the static equivalent to the runtime API.
We know it will be useful. And how.
Yes, for square
specifically they should annotate this with some form of CanMul
. But as we've been discussing in #32 writing this type is non-trivial and array_api_typing
should probably be the space where we vendor something like
NumArrayCanMul = CanMulSelf[..., ...] # fills in the complex types
BoolArrayCanMul = CanMulSelf[..., ...] # The bool considerations we discussed
so that higher-level functions that require many parts of the Array API can do
def main():
x = xp.array(...)
...
... square(...)
HasNDim, | ||
HasShape, | ||
HasSize, | ||
HasTranspose, |
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 HasTranspose
restricts this to 2d arrays
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.
Does it? I don't know of any array object that doesn't have this method, regardless of the value's dimensionality.
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.
.transpose()
is only defined for 2d arrays, so we should expect array libs to follow that, and we should therefore expect there to be annotations like
class SomeArray[ShapeT, DTypeT]:
# --snip--
def transpose(self: SomeArray[Rank2, DTypeT]) -> SomeArray[Rank2, DTypeT]: ...
So a SomeArray[Rank1]
won't be assignable to xpt.Array
.
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.
That sounds great.
It sounds like there are two interpretations of how this should be represented statically given the API spec: "The array instance must be two-dimensional. If the array instance is not two-dimensional, an error should be raised."
- That it statically determines the shape. This focuses on the 1st sentence.
@property
def T(self: SomeArray[Rank2, DTypeT]) -> SomeArray[Rank2, DTypeT]: ...
- that it shadow the runtime. This focuses on the 2nd sentence.
@overload @property
def T(self: SomeArray[Rank0 | Rank1, DTypeT]) -> Never: ...
@overload @property
def T(self: SomeArray[Rank2, DTypeT]) -> SomeArray[Rank2, DTypeT]: ...
@overload @property
def T(self: SomeArray[RankGE3, DTypeT]) -> Never: ...
(or if we don't have shape typing)
def T(self: SomeArray) -> SomeArray | Never: ... # yes, the Never is ignored.
This was written more with the 2nd interpretation in mind, but I'm very happy to accept the 1st interpretation and remove T
from the default Array object, so long as we still include some form of HasTranspose
Protocol for people to use.
I do think there is then interesting discussion about that Protocol ....
Should we vendor 2 forms? one for each interpretation? That will help people when building their own intersection types.
... | ||
|
||
|
||
class HasNDim(Protocol): |
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.
Is there a situation where you'd wanna use HasNDim
over e.g. HasShape
? Otherwise we probably should keep this private, given that
There should be one-- and preferably only one --obvious way to do it.
Although that way may not be obvious at first unless you're Dutch.
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.
If you're checking for the attribute ndim
.
Not sure I'm understanding this comment. Ndim is in the Array API spec...
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.
Well, my point is that I think we should only provide users with protocols that help them annotate their array-api code. Otherwise, it'll just be confusing for the users, and a waste of time for us.
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.
Oh. As the Array API is the intersection of the array libraries, IMO pretty much everything is useful.
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.
Yea of course, everything in the array-api is designed for a reason. But all of those reasons apply in the runtime world. And our goal is to provide an API for the static-typing world, which is but a shadow of the runtime one, where only a subset of the API has practical use.
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.
That's true, I'm just not sure we could really know which of the specific methods are/aren't useful for people in every situation. I agree we get more control over what goes into Array
, but shouldn't we provide a Protocol for each individual method and attribute?
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.
but shouldn't we provide a Protocol for each individual method and attribute?
We're not a stub library, so no, I don't think we should.
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.
We do need a stub library... Maybe that's the easier thing to do first.
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.
We do need a stub library... Maybe that's the easier thing to do first.
Stub libraries only apply to individual libraries. Stub libraries also can't be overridden or something, so libraries that have other methods besides the one from the array-api won't be able to use it. So I don't see how that would work (even though I'm all about writing stubs, as you know)
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.
True. I guess I mean a direct method-by-method set of Protocols for the array API, like is in this PR.
I know this is what many people want, as a baseline at the very least. IMO this is the place for it.
a5e3d5b
to
27ef62d
Compare
Signed-off-by: nstarman <[email protected]>
Signed-off-by: nstarman <[email protected]>
Signed-off-by: nstarman <[email protected]>
Signed-off-by: Nathaniel Starkman <[email protected]>
Signed-off-by: nstarman <[email protected]>
27ef62d
to
c65c5d5
Compare
@jorenham. I can see your points about some of the dangers of building the Array intersection protocol from all these method-by-method protocols before we've figured out shape typing, etc. |
Requires #32. I'll rebase when that's in.Now preceding #32.