Skip to content

[TYP] Fix overload of pyplot.subplots #28518

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

Merged
merged 5 commits into from
Jul 19, 2024

Conversation

adamjstewart
Copy link
Contributor

PR summary

Fixes #27001 (comment)

My solution is to have 3 possible cases:

  1. squeeze=True, nrows=1, ncols=1: Axes
  2. squeeze=True: np.ndarray
  3. squeeze=False: np.ndarray

1 and 3 already existed. 2 is hopefully treated as a fallback by mypy for when either nrows or ncols is not 1. All of my testing with reveal_type works as expected, but I would love suggestions for how to formally test this in CI so it doesn't happen again.

PR checklist

@adamjstewart
Copy link
Contributor Author

Interesting, mypy is fine with this approach but stubtest is not:

error: not checking stubs due to mypy build errors:
lib/matplotlib/figure.pyi:95: error: Overloaded function signatures 1 and 2 overlap with incompatible return types  [misc]
lib/matplotlib/pyplot.py:1566: error: Overloaded function signatures 1 and 2 overlap with incompatible return types  [misc]

Is it possible to say all integers except 1? If not, we may have to go back to partially dynamic typing as suggested in #27001 (comment).

@adamjstewart
Copy link
Contributor Author

Let's see if we can keep the .py code specific and only loosen the restrictions on the .pyi stubs.

@ksunden
Copy link
Member

ksunden commented Jul 11, 2024

I think we may be able to get by by simply changing only the "fallback" case from having a union return type to just being Any, but keeping the more specific versions to provide the benefits of having type hints when we can.

The truth is that the type hint is not wrong... it is, to the best of our ability within the type system, giving an accurate description of the return type, which we cannot say whether it will be a single Axes or an ndarray based on the type...

I specifically called out potentially wanting Any for this case to avoid union types on returns in #27001 (review)

@adamjstewart
Copy link
Contributor Author

adamjstewart commented Jul 11, 2024

I think we may be able to get by by simply changing only the "fallback" case from having a union return type to just being Any, but keeping the more specific versions to provide the benefits of having type hints when we can.

Thanks, I think this is a better solution than what I came up with.

The truth is that the type hint is not wrong...

This is where I disagree. It is true that the return type can be Axes or np.ndarray, but it is not correct to say that the return type is Axes | np.ndarray.

we cannot say whether it will be a single Axes or an ndarray based on the type...

This is referred to as dynamic typing. In Python Any is used "to indicate that a value is dynamically typed." Using a union as a return type implies that the return value can be of either type, and that all uses of that function must be able to handle both. Unions are almost never used in return types, only in parameter types.12

Footnotes

  1. https://github.com/python/mypy/issues/1693

  2. https://github.com/python/typing/issues/566

@adamjstewart
Copy link
Contributor Author

Is anyone able to review this? Would love to get this into 3.9.2.

@timhoffm timhoffm added this to the v3.9.2 milestone Jul 19, 2024
@timhoffm timhoffm merged commit 087a142 into matplotlib:main Jul 19, 2024
44 checks passed
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Jul 19, 2024
QuLogic added a commit that referenced this pull request Jul 20, 2024
…518-on-v3.9.x

Backport PR #28518 on branch v3.9.x ([TYP] Fix overload of `pyplot.subplots`)
@kalvdans
Copy link

I can confirm that this change fixes mypy error in my job's codebase.

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.

4 participants