-
Notifications
You must be signed in to change notification settings - Fork 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
TYP: Type annotations overhaul, part 2 #291
base: main
Are you sure you want to change the base?
Conversation
The next Consortium meeting is currently scheduled for the 3rd of April. In view of #257 (comment), it'd be great if someone brings up the topic of this and gh-288. Ideally, someone with an actual understanding and a horse in the race (so not me). |
As I've stated in #257 (comment), this is at odds with the typing spec. This has nothing to do with the array-api itself, it's just a matter of following the rules of Python itself. It's ok if you disagree with those, and I even understand why you would. But then again, we're not the ones that are making these rules. The appropriate platform to discuss this is at https://discuss.python.org/c/typing/32. If you decide to open a thread there, you'd find me by your side @ev-br . |
I fully agree that this is at odds with the typing spec, but unless you can convince @ev-br to follow it, this PR makes the whole |
Do you guys think it would help to make list of points at least so that this controversy can be understood by everyone? From what I understand: I think everyone agrees that there should be consistency between the type annotations in:
and that type annotations should be correct, and as clear as possible. Using
Using
There is also the question of how to annotate something like def maximum(x: int | Array, y: int | Array): with an understanding for human readers that def maximum(x: optype.Just[int] | Array, y: optype.Just[int] | Array): which is clear to type checkers and human readers that Did I miss anything? Once we have everything down, it may be possible to work around some of the concerns. For example, if documentation is the issue, we may be able to generate documentation with the types exploded, or to add a note to every affected function. My personal suggestion is to post this to the Array API forum, and ping some of the Python typing experts (like Shantanu or Jelle). |
Thanks for the write-up!
It also causes inconsistent behaviour between type-checkers: #257 (comment) |
At least one of some novices :-) has no idea what |
Didn't mean that pejoratively, and hope it didn't come across that way. I'll reword just in case. Please let me know if my edit is more acceptable. (All I meant was that if you've read/written a lot of type annotations your mind gets used to seeing Is your point that:
|
complex | np.complex128 | float | np.float64 | int | bool ? Also note that the array-api promotion rules are separate from nominal subtyping relations: An |
Don't worry, my comment was tongue-in-cheek. We get all too serious discussing a too serious matter. More seriously though, I sense there's a POV difference. You seem to expect that everybody is either a typing practitioner or a novice which needs to get to speed with types.
Yes. As long as annotations are visible to users and developers, I believe they should be consistent and no actively misleading. Which, I believe, using
Note I'm only discussing Your point is maybe about the full-fledged numpy types. That I have no opinion other than "please tuck it somewhere where human users opt in to see them". So maybe the answer here is similar: if indeed,
Then let's just tuck all annotations somewhere |
No can do; once the typing annotations just say
|
It's Python: you can do anything you want 😄 . You can have a decorator or a method list that describes how you want it to be exploded. Anyway, in the case you list, you should completely explode it for consistency if that's what you want the docs to look like. |
I don't think anyone "needs to get to speed". I do think that a consequence of working with the Python typing system is that your intuition changes. Maybe it would help, to just look at what it means for def pow(base: float, exp: complex) -> Any: ...
This encodes how weird it is in the Python world for such a function to only work with floats. The type system itself is nudging you to either make it work for integers and floats, or go through extreme measures to annotate this function to only work with floats. I think this is a good policy because it means that as a caller, you shouldn't have to think too much about whether you have to call the function with I understand that you feel like the above annotation for
Sure, but that "somewhere" is the code, right? I agree that all of the code in all of the Array API projects should be consistent. But it should also follow the way typing works in Python too, right? I think if people on the Array API team feel that this makes the documentation too misleading for users, then I think they should explore ways of making the documentation annotations broader. |
FWIW, the Array API doesn't have any use case where a function accepts float but not int, or complex but not float. |
Would you mind clarifying for me? Which cases are these? I'm not sure I understood the summation examples. |
For example, xp.maximum is one of several Array API functions that requires its inputs to be numeric - it accepts An (in my head) straightforward solution would be to just standardize bool->int type promotion:
This would make a lot of sense at runtime, namely by enabling the commonplace pattern The downside I see is that you would need to think through what some operations do: is |
Exactly. Even I'm fairly sure that we do not want to change this decision. |
Thanks for explaining! I agree that Python scalar Yes, it would have been better had Python not been designed with this parent class, but this is unlikely to change. |
To add to what already said:
The problem is not in Python itself, it's with static type checkers.
Absolutely. Just not in the function signature which is the most visible place of all. |
In the last few comments, we're talking about
Why not? Who's going to look at the function signature except developers who understand type annotations? |
Type checkers don't have to obey the runtime's inheritance relationship. They are already disregarding it for int < float < complex. |
I think there's a big difference between pretending that there's relationship, and forgetting about one. There has been a prevailing opinion among type checker authors that legal code should be accepted. This informed a lot of debates. For example, when MyPy started accepting things that might implement the sequence protocol as iterable (despite having no Since On the other hand, pretending that If you really want type checkers to change and start blocking the usage of |
Anybody. As already stated above, I do not buy the idea that "understand type annotations" is a prerequisite.
Thanks for the suggestion Joren! I believe the best chance for an upstream discussion to be productive is if it starts based on a deliberation by the Array API Consortium. This thread starts getting long and repetitive. I'm going to recap my position (the majority of points were also made by multiple people here), and take a step back from the thread:
This is because
In case it is not obvious, Consortium meetings are open to all interested parties. Please bring the topic in one of the Consortium meetings. |
Given this, can we merge this PR as well as data-apis/array-api-strict#135? It's much easier to revert the relevant bits at a later date if the consortium decides that the short form is the way to go, than to keep these PRs open indefinitely. |
The main thing that keeps me from landing these is #257 (comment) |
@ev-br Thanks for clarifying your position. I've added your point about Edit: Just FYI:
It is not an implementation detail; |
Unless you use import optype as op
def assert_int(x: op.Just[int]) -> int:
assert type(x) is int
return x
assert_int(42) # accepted
assert_int(False) # rejected |
Follow-up to #257, reverting leftover controversial changes re. python scalar data types (see discussion on the other PR).
@ev-br