Skip to content
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 #135

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

TYP: type annotations #135

wants to merge 11 commits into from

Conversation

crusaderky
Copy link
Contributor

@crusaderky crusaderky commented Mar 21, 2025

  • Implement thorough static type annotations
  • Align type annotations conventions to array-api-compat (TYP: Type annotations overhaul, part 1 array-api-compat#257)
  • Add py.typed
  • Manually run mypy and fix most typing issues
  • Manually run isort (one-off)
  • Fix bug where iinfo and finfo would return a numpy dtype instead of a DType

Out of scope

  • Fix all mypy errors (some highlight material issues that deserve their own PRs)
  • Add mypy to CI
  • Make pyright happy
  • tests module

@ev-br
Copy link
Member

ev-br commented Mar 21, 2025

Can ignore python 3.9, cf #129

Comment on lines 314 to 315
# Note: indexing was 'str' in <=2024.12
def meshgrid(*arrays: Array, indexing: str = "xy") -> list[Array]:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure if this is inconsequential enough that I should just go on and define it as Literal here?

@crusaderky
Copy link
Contributor Author

Can ignore python 3.9, cf #129

Unless you plan to merge #129 now, I can't ignore 3.9 as it would make all CI to fail.

@crusaderky
Copy link
Contributor Author

CC @jorenham @NeilGirdhar

@jorenham jorenham self-requested a review March 21, 2025 17:04
Copy link
Member

@ev-br ev-br left a comment

Choose a reason for hiding this comment

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

Thanks for the PR Guido!

I went through up to _linalg.py before running out of steam for the week. In general it looks great, my main gripe is about annotations for python scalars:

  • several look like global search-and-replace : floordiv, __gt__ just do not accept complex
  • I'm -1 for changing bool | int | float into just float.

@@ -184,7 +193,9 @@ def __array__(self, dtype: None | np.dtype[Any] = None, copy: None | bool = None
# spec in places where it either deviates from or is more strict than
# NumPy behavior

def _check_allowed_dtypes(self, other: bool | int | float | Array, dtype_category: str, op: str) -> Array:
def _check_allowed_dtypes(
self, other: Array | complex, dtype_category: str, op: str
Copy link
Member

Choose a reason for hiding this comment

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

ints, bools and floats are allowed. I get that a float is-a complex but nonetheless. As long as this is for a human reader more than for a linter, let's be more descriptive. Here and elsewhere.

Choose a reason for hiding this comment

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

As long as this is for a human reader more than for a linter

I think @jorenham is right that this has a lot of drawbacks. Once you get used to annotations, you know that complex subsumes all of those options. If you want to make things clear for novices, why not use a docstring?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's the point though, none of this is for a human reader that isn't pushing a PR to this repo, save for the public functions in _flags.py!

https://data-apis.org/array-api-strict/api.html

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, no. If the tooling is not ready, we don't use it until it is. Please roll it back, here and in the -strict PR.

Copy link

@NeilGirdhar NeilGirdhar Mar 21, 2025

Choose a reason for hiding this comment

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

What do you mean by the "the tooling is not ready"? Python typing will probably never alter the definition of these basic types, right?

Choose a reason for hiding this comment

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

What do you mean by the "the tooling is not ready"? Python typing will probably never alter the definition of these basic types, right?

That's right.

Copy link

@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.

Good stuff! I left a couple of suggestions; do with it what you want 👌🏻

@@ -184,7 +193,9 @@ def __array__(self, dtype: None | np.dtype[Any] = None, copy: None | bool = None
# spec in places where it either deviates from or is more strict than
# NumPy behavior

def _check_allowed_dtypes(self, other: bool | int | float | Array, dtype_category: str, op: str) -> Array:
def _check_allowed_dtypes(
self, other: Array | complex, dtype_category: str, op: str

Choose a reason for hiding this comment

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

What do you mean by the "the tooling is not ready"? Python typing will probably never alter the definition of these basic types, right?

That's right.

@jorenham
Copy link

I took the liberty of creating a label for static typing stuff and painted it black because that's how I prefer my coffee (and my metal)

ev-br
ev-br previously requested changes Mar 26, 2025
Copy link
Member

@ev-br ev-br left a comment

Choose a reason for hiding this comment

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

Let me record my -1 on bool | int |float until data-apis/array-api-compat#257 (comment) is done.

@crusaderky
Copy link
Contributor Author

Let me record my -1 on bool | int |float until data-apis/array-api-compat#257 (comment) is done.

Can we please find a minimum common ground that allows us to merge this PR?
I've just changed all scalar types to match exactly the Array API docs. That means explicit bool | int | float | complex, bool | int, and int | float | complex.

@ev-br
Copy link
Member

ev-br commented Mar 26, 2025

To repeat, my only strong feelings are on

  • consistency of annotations and signatures with the Array API docs and stubs, and
  • int | bool | float,
    in this order.

With these two items being there, I'm declaring my expertise on typing to run out, the typing itself being considered mostly harmless; I'm thus dismissing my request for changes and bow out. As long as my boundary conditions are not exceeded, I'm happy with whatever y'all converge on.

@ev-br ev-br dismissed their stale review March 26, 2025 16:40

Request addressed.

@crusaderky
Copy link
Contributor Author

This is ready for final review.

@crusaderky crusaderky marked this pull request as ready for review March 26, 2025 16:47
@crusaderky
Copy link
Contributor Author

I believe there should not be any oustanding comments?

@lucascolley
Copy link
Member

lucascolley commented Mar 29, 2025

Trying to push things forward hat on:

It sounds like both @crusaderky and I would be happy to keep int | bool | float in for now given @ev-br's concern, with a view to eventually removing them once we have array-api-typing up and running and readable/documented aliases, which make sense appearing both in the 'real' annotations and on the spec pages.

Would merging @crusaderky's 2 PRs disrupt your work at data-apis/array-api-compat#288 @jorenham ?

  • consistency of annotations and signatures with the Array API docs and stubs, and
  • int | bool | float,
    in this order.

I haven't read the entirety of all of these threads, but are there any outstanding concerns about consistency apart from the int | bool | float situation?


Opinion hat on:

For me, there is no significant consistency concern about int | bool | float, for reasons already stressed in this thread: these annotations are tool/developer facing, not user facing. I think it is fairly obvious to developers adopting the spec that the spec pages are the source of truth. Following SciPy's lead, we should distinguish between documented types (for docs readability) and the actual type annotations, keeping in mind that we can hope for these to converge somewhat with array-api-typing. In this sense some 'inconsistency' is actually desirable.

If the concern is that inconsistency between the spec saying int | float and an annotation in (say) array-api-strict saying float will confuse array-api-strict developers, I think this is best remedied in the relevant contributor guides, rather than holding up the typing experts from making the much-desired improvements.

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

Successfully merging this pull request may close these issues.

5 participants