Skip to content

✨: add CanArrayX protocols #32

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

Open
wants to merge 29 commits into
base: main
Choose a base branch
from
Open

Conversation

nstarman
Copy link
Collaborator

@nstarman nstarman commented Jun 22, 2025

No description provided.

@nstarman
Copy link
Collaborator Author

Ok This PR is doing too much. Let me pair it down to just a few Protocols and do the rest as a series of followups.

nstarman added 2 commits June 23, 2025 09:41
Support unary minus operator.

Signed-off-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
nstarman added 7 commits June 23, 2025 09:48
Support addition operator for array classes.

Signed-off-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
Support subtraction operator for array classes

Signed-off-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
Support multiplication operator for array classes

Signed-off-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
Support true division operator for array classes
Support floor division operator for array classes
Support modulo operator for array classes
Support power operator for array classes
@nstarman nstarman force-pushed the has_x branch 2 times, most recently from b47800d to 96067a4 Compare June 23, 2025 18:34
nstarman added 13 commits June 23, 2025 12:11
Support in-place addition operator for array classes
Support in-place subtraction operator for array classes
Support in-place multiplication operator for array classes
Support in-place true division operator for array classes
Support in-place floor division operator for array classes
Support in-place power operator for array classes
Support in-place modulo operator for array classes
Support right addition operator for array classes

Signed-off-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
Support right subtraction operator for array classes
Support right multiplication operator for array classes
Support right division operator for array classes

Signed-off-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
Support right floor division operator for array classes
Support right power operator for array classes
Support right modulo operator for array classes

Signed-off-by: Nathaniel Starkman <nstarman@users.noreply.github.com>
@nstarman nstarman marked this pull request as ready for review June 23, 2025 21:59
@nstarman nstarman requested a review from jorenham June 23, 2025 21:59
@nstarman
Copy link
Collaborator Author

Ping @NeilGirdhar, given related discussions.

@nstarman nstarman changed the title ✨: add HasArrayX protocols ✨: add CanArrayX protocols Jun 23, 2025
@@ -0,0 +1,502 @@
__all__ = (
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure which should be public. All?

...


class CanArrayAdd(Protocol):
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was thinking about parametrizing by dtype. Self, other, output. Bit of a mess. Maybe tackle parametrizing as a followup?

@nstarman
Copy link
Collaborator Author

Should all the Protocols inherit from HasArrayNamespace?
Also should it be rename to CanArrayNamespace ?

@NeilGirdhar
Copy link
Contributor

NeilGirdhar commented Jun 23, 2025

Should all the Protocols inherit from HasArrayNamespace?
Also should it be rename to CanArrayNamespace ?

I don't know what Joren will say, but I would guess no and no? (I think you got it right in this PR?)

Also, I'm guessing you're aware that int | float is float, and you're intentionally specifying both?

@nstarman
Copy link
Collaborator Author

nstarman commented Jun 24, 2025

don't know what Joren will say, but I would guess no

My thought was for building stuff like

class Positive(Protocol):
    def __call__(self, array: CanArrayPos, /) -> CanArrayPos: ...

is wrong.

It should be something like

class Positive(Protocol):
    def __call__(self, array: HasArrayNamespace, /) -> HasArrayNamespace: ...

But I think we want

class Positive(Protocol):
    def __call__(self, array: CanArrayPos, /) -> HasArrayNamespace: ...

Which I think works best if it's

class CanArrayPos(HasArrayNamespace, Protocol): ...

Also, I'm guessing you're aware that int | float is float, and you're intentionally specifying both?

Yes. :).

@NeilGirdhar
Copy link
Contributor

I see, you're kind of using it as a poor man's intersection?

Also, I'm guessing you're aware that int | float is float, and you're intentionally specifying both?

Yes. :).

Okay, is that because you're going to generate some documentation from these annotations? Or you find it less confusing?

Also, are you going to add complex to the union?

@nstarman
Copy link
Collaborator Author

Okay, is that because you're going to generate some documentation from these annotations? Or you find it less confusing?

It's for 2 reasons: the array api does it in their docs and because I think the Python numerical tower is a mess and since ints and floats aren't subclasses of each other, it makes little sense for them to be interchangeable at the static type level. 😤😆

Also, are you going to add complex to the union?

Worth discussing. The array api does not.

@NeilGirdhar
Copy link
Contributor

NeilGirdhar commented Jun 24, 2025

It's for 2 reasons: the array api does it in their docs

The docs are that way to help beginners who might be confused. (At least that was the argument that was presented.) But you aren't expecting beginners to read your code, are you?

And, you aren't using this repo to build docs?

The downside of populating the unions unnecessarily is overcomplicated type errors. So from a user standpoint, I think this is worse.

From a developer standpoint, it's a matter of taste. Personally, I think more succinct is easier to understand.

because I think the Python numerical tower is a mess and since ints and floats aren't subclasses of each other, it makes little sense for them to be interchangeable at the static type level.

As much as you might like to turn back time and change the typing decisions that were made, the fact is that the static type int is a subclass of float as far as type checkers are concerned, and that will not change for the foreseeable future.

I think I understand what you're doing and why. I spent years writing if x != 0 for a similar reason. But I think this is a fact that you just have to accept even if you dislike it.

Worth discussing. The array api does not.

Does it not?

array.__add__(other: int | float | complex | array, /) → array

Have I misunderstood the documentation?

@nstarman
Copy link
Collaborator Author

nstarman commented Jun 24, 2025

Ah. We're building towards v2021 first.
A release branch for every major version.
The versions have almost been entirely additive, so it's not too onerous.
This also makes backporting easier.

Copy link
Member

@jorenham jorenham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It might be easier to use optype for this, as it already provides single-method generic protocols for each of the special dunders:

https://github.com/jorenham/optype/blob/master/optype/_core/_can.py

There's even documentation: https://github.com/jorenham/optype#binary-operations

And of course it's tested and thoroughly type-checked and stuff

@nstarman
Copy link
Collaborator Author

nstarman commented Jun 24, 2025

Sounds good to me...
It's good to have in-house expertise.

CleanShot 2025-06-24 at 10 22 36@2x

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants