Skip to content

Conversation

@lispandfound
Copy link
Contributor

Addresses #991

@github-actions
Copy link

github-actions bot commented Nov 21, 2025

mypy_primer results

No ecosystem changes detected ✅

@jorenham jorenham changed the title 🐛 use generic return type for DisjointSet.__getitem__ 🐛 cluster.hierarchy: use generic return type for DisjointSet.__getitem__ Nov 21, 2025
@jorenham jorenham added this to the 1.16.3.1 milestone Nov 21, 2025
@jorenham jorenham linked an issue Nov 21, 2025 that may be closed by this pull request
@jorenham
Copy link
Member

Thanks for this!

Could you also add a quick little type-test for this in https://github.com/scipy/scipy-stubs/tree/master/tests , to confirm that it's now working as intended? It would also help me understand it a bit better haha.

@lispandfound
Copy link
Contributor Author

lispandfound commented Nov 21, 2025

Yes I will do this, and I will also address the incorrect type in __init__ you noted in the issue.

@lispandfound
Copy link
Contributor Author

...and an issue with __contains__ accepting object when it should accept _T

@lispandfound
Copy link
Contributor Author

lispandfound commented Nov 21, 2025

I made a small change to the signature to leave _T unbound in the case where no elements are passed:

_T = TypeVar("_T", bound=op.CanHash)

But I now see that the test matrix fails for numpy<2.2, is this expected? If so, I will revert that change.

@jorenham
Copy link
Member

I made a small change to the signature to leave _T unbound in the case where no elements are passed:

_T = TypeVar("_T", bound=op.CanHash)

But I now see that the test matrix fails for numpy<2.2, is this expected? If so, I will revert that change.

Removing that default would break backwards compatibility. Using default=Any instead of object is what I'd do here.

@jorenham
Copy link
Member

jorenham commented Nov 21, 2025

But I now see that the test matrix fails for numpy<2.2, is this expected?

The problem is that before NumPy 2.1, there was an issue in NumPy's bundled stubs where the __hash__ methods was missing for certain scalar types like int32. That mistake was easy to make though, because the __hash__ method "disappears" if you only provide an __eq__ method, without also providing an explicit __hash__ method. Pyright, unlike mypy, also applies these cpython runtime semantics to the stubs. So that's why pyright is selectively complaining here.

It's easy to work around though, i.e. set _T's bound to something like bound=op.CanHash | np.generic.

@lispandfound
Copy link
Contributor Author

Ok I think this is now in a reviewable state again.

@jorenham
Copy link
Member

This is looking very good; thanks Jake!

@jorenham jorenham merged commit 7433b5e into scipy:master Nov 23, 2025
21 checks passed
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.

DisjointSet.__getitem__ has incorrect type

2 participants