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
ENH: Extend np.add ufunc to work with unicode and byte dtypes #24858
Conversation
One question to ask ourselves here is whether we want to also implement loops for mixed dtypes like |
I don't think the ufunc fastpath has anything to offer once |
Here's another run of the benchmarks.
|
c2a6975
to
1f3a508
Compare
I agree, we shouldn't assume the encoding of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a couple of fly-by comments, since I had a look. But overall, it seems good.
This appears to be ready to merge. Are there any more review comments for me to address? |
It needs a release note. Look in |
@charris Added a minimal release note. Should it be more comprehensive than that? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments, maybe @ngoldbaum can have a quick look, generally should be good to go though, but I would like the resolve dtypes to be clarified as "this doesn't make sense, but we need it for the DType class/kind".
Also, tests for things that were clearly buggy but fixed in later review would be good.
Co-authored-by: Sebastian Berg <sebastian@sipsolutions.net>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one super minor nit otherwise this looks great!
Thanks @lysnikolaou! |
No description provided.