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

🏷️ ufunc annotations for logical_{not,and,or,xor} #324

Merged
merged 8 commits into from
Mar 18, 2025

Conversation

guan404ming
Copy link
Contributor

@guan404ming guan404ming commented Mar 16, 2025

Towards #230

@guan404ming guan404ming changed the title 🏷️ ufunc annotations for logical-{not,and,or,xor} 🏷️ ufunc annotations for logical-{not,and,or,xor} Mar 16, 2025
@guan404ming guan404ming marked this pull request as ready for review March 16, 2025 07:19
@jorenham jorenham added this to the v2.2.x.0 milestone Mar 16, 2025
@jorenham jorenham changed the title 🏷️ ufunc annotations for logical-{not,and,or,xor} 🏷️ ufunc annotations for logical_{not,and,or,xor} Mar 16, 2025
@guan404ming
Copy link
Contributor Author

@jorenham I see the difference between them. Should I create a new class for the logical-related functions, or is there an existing class in the codebase that I might have missed? Just wondering if there's something already available before I go ahead with a new one. 😊

@jorenham
Copy link
Member

jorenham commented Mar 16, 2025

@jorenham I see the difference between them. Should I create a new class for the logical-related functions, or is there an existing class in the codebase that I might have missed? Just wondering if there's something already available before I go ahead with a new one. 😊

I don't that there's anything like that in the numtype codebase yet. I also couldn't find anything salvageable for this in scipy-stubs (https://github.com/scipy/scipy-stubs/blob/master/scipy-stubs/special/_ufuncs.pyi).

Kudo's for your "don't reinvent the wheel" mindset, BTW πŸ‘ŒπŸ»


FYI, the default callable protocol includes a workaround for ->O cases like these:

@overload
def __call__(
self,
x1: ArrayLike,
x2: ArrayLike,
/,
out: None = None,
dtype: DTypeLike | None = None,
**kwargs: Unpack[_Kwargs3],
) -> _OutT_co | NDArray[np.object_]: ...

But that's a rather awkward solution that should only be seen as a "last-result fallback placeholder workaround default" thing (but this PR shows that I'm preaching to the choir).

@guan404ming
Copy link
Contributor Author

Not kind of sure about the implementation if great or not. I try my best to handle all kind of situations~
Pls help me review merci @jorenham

@jorenham jorenham self-requested a review March 16, 2025 21:11
Copy link
Member

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

This is a pretty difficult signature to annotate, so it might take a couple of tries to get it right.

It's also no problem if you're not able to figure it out yet. So if that's the case then I wouldn't mind helping out here.

@guan404ming
Copy link
Contributor Author

guan404ming commented Mar 17, 2025

Here’s a simple note I learned from this discussion:

  1. np.object_ doesn’t really exist as a scalar – it just returns the input type. It works as dtype=object in arrays, but not as a standalone type.
  2. not _ always returns bool, so the return type should be bool, not np.object_.
  3. Mypy has a bug that makes it hard to properly annotate np.object_behavior.
  4. __array_ufunc__ can return anything πŸ¦†
  5. For dtype=np.object_, the return type should be NDArray[np.object_] | something you want, since np.object_ doesn’t work as a scalar.
  6. Overloads should be ordered carefully, with more specific ones first.
  7. DTypeLike adjustments are needed to avoid unwanted np.object_ inference.

Big thanks for the explanation! This is what I learned from u, and I noted it down so others can also share this knowledge. πŸš€

merci @jorenham

Copy link
Member

@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 progress πŸ’ͺ🏻.

Here's the next iteration of comments, hopefully they make sense.

@guan404ming guan404ming requested a review from jorenham March 18, 2025 08:17
@guan404ming guan404ming force-pushed the ufunc-annotations-for-logical branch 2 times, most recently from a3cf8a1 to f7df097 Compare March 18, 2025 11:43
@guan404ming guan404ming force-pushed the ufunc-annotations-for-logical branch from f7df097 to a3cf8a1 Compare March 18, 2025 11:54
@guan404ming guan404ming force-pushed the ufunc-annotations-for-logical branch from a3cf8a1 to ecb6397 Compare March 18, 2025 12:21
@guan404ming guan404ming requested a review from jorenham March 18, 2025 12:44
@guan404ming
Copy link
Contributor Author

Updated~ Thanks for comments learn a lots

@guan404ming
Copy link
Contributor Author

Oh sorry, my fault 🀦

@guan404ming guan404ming requested a review from jorenham March 18, 2025 13:04
Copy link
Member

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

Almost there!

Co-Authored-By: Joren Hammudoglu <[email protected]>
@guan404ming
Copy link
Contributor Author

Big thanks again!

@guan404ming guan404ming requested a review from jorenham March 18, 2025 16:51
Copy link
Member

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

I didn't notice this one before; sorry about that. But that one aside, I've got nothing to complain about anymore πŸ‘ŒπŸ»

@guan404ming
Copy link
Contributor Author

Updated, thanks for helping out and taught me a lots!

@guan404ming guan404ming requested a review from jorenham March 18, 2025 17:39
@jorenham jorenham merged commit 35b02ec into numpy:main Mar 18, 2025
21 checks passed
@jorenham
Copy link
Member

In it goes β€” Thanks Guan-Ming πŸŽ‰

@guan404ming guan404ming deleted the ufunc-annotations-for-logical branch March 19, 2025 00:28
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.

2 participants