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

Make type annotations for NumPy arrays more specific #1358

Merged
merged 5 commits into from
Jan 15, 2025

Conversation

IvanIsCoding
Copy link
Collaborator

Related to #1352

This is trying to address some complaints from pyright infering the NumPy array as Unknown.

@IvanIsCoding IvanIsCoding requested a review from mtreinish January 8, 2025 23:47
@coveralls
Copy link

coveralls commented Jan 9, 2025

Pull Request Test Coverage Report for Build 12776415127

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 95.83%

Totals Coverage Status
Change from base Build 12773634822: 0.0%
Covered Lines: 18384
Relevant Lines: 19184

💛 - Coveralls

@barakatzir
Copy link

barakatzir commented Jan 9, 2025

I don't know if you want yo add it to this PR, but pyright also complains about numpy.dtype needing parameter annotations.

@IvanIsCoding
Copy link
Collaborator Author

I don't know if you want yo add it to this PR, but pyright also complains about numpy.dtype needing parameter annotations.

I annotated it with np.dtype[np.generic] but to be honest I find that par counter-intuitive.

@barakatzir
Copy link

I don't know if you want yo add it to this PR, but pyright also complains about numpy.dtype needing parameter annotations.

I annotated it with np.dtype[np.generic] but to be honest I find that par counter-intuitive.

You can set it to np.dtype[Any] which matches the new return type npt.NDArray[Any]. It sounds more intuitive to me.

@mtreinish mtreinish added this to the 0.16.0 milestone Jan 14, 2025
Copy link
Member

@mtreinish mtreinish left a comment

Choose a reason for hiding this comment

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

Overall this LGTM, just one small inline question about the hint for __array__

Comment on lines +1084 to +1085
self, dtype: np.dtype[Any] | None = ..., copy: bool | None = ...
) -> npt.NDArray[Any]: ...
Copy link
Member

Choose a reason for hiding this comment

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

Do we want to use a generic/typevar here? Since the Any for the dtype argument needs to match the dtype inside the returned array?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

For the record we ignore the dtype at runtime. However, I cannot annotate the ndarray with _T as that would be incorrect. There is a mapping from _T to numpy types that we’d need to manually add and that is a lot of work.

Also, this method exists to support np.array calls no one should call it directly. I think NumPy type stubs will mask this call anyway

@mtreinish mtreinish added this pull request to the merge queue Jan 15, 2025
Merged via the queue into Qiskit:main with commit b44272f Jan 15, 2025
31 checks passed
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.

4 participants