ENH: Add rest of unary ufuncs for unicode/bytes dtypes #25791
Conversation
lysnikolaou
force-pushed
the
string-ufuncs-is-case
branch
from
6ca0dae
to
fdb3062
Compare
lysnikolaou
changed the title
|
Refactoring all the unary loops to call a single helper makes sense to me, looks like a nice cleanup which I will do in stringdtype after this. |
Well, that was your design in stringdtype, which I kinda replicated. So you've done this already here! |
|
@ngoldbaum I did a minor refactoring of the test classes/methods, but I'm not sure if that's better for you or not. Could you please have a look? |
There was a problem hiding this comment.
The tests look good now, thanks for doing that refactor! I suspect a large fraction of the xfails in numpy can be safely removed and probably should have been specified with strict=True so I'm trying to make sure that happens going forward when it's possible (e.g. a failure that only happens on some architectures can't be strict).
|
OK, I'll go ahead and add implementations for stringdtype in this PR, I think that will be pretty straightforward. |
|
I had a quick look earlier and it seemed a case where #25796 will make the PR a lot smaller. So, I'd prefer to review after that... Independently, it really speaks for the whole approach that it is now so relatively easy to add loops for all the string types. Kudos! |
|
FYI I'll reimplement the unary ufuncs with the new approach after #25775 has been merged. |
There was a problem hiding this comment.
@lysnikolaou - I think there still is a large gain to be made by not generating separate inner loops for each of the boolean query functions but rather passing in the method that should be called - see inline comment.
|
Hmm, looks like I was reviewing the wrong PR, misreading that you had already changed it, sorry!! |
|
This is ready for review now. |
lysnikolaou
force-pushed
the
string-ufuncs-is-case
branch
from
96646fb
to
2bc79d7
Compare
|
It looks like the windows pypy failures are real, let me know if you want a hand debugging that. |
Oh, I didn't see that. If you have access to a Windows machine and can spare some cycles on this, it'd really help me out, cause I don't have access to Windows right now (we'd have to wait for a bit until I do). |
| }; | ||
|
|
||
| for (int i = 0; i < 9; i++) { | ||
| if (i < 7) { // isdecimal & isnumeric do not support ASCII |
There was a problem hiding this comment.
I don't really mind either way, but is there a reason that isdecimal and isnumeric are not supported?
There was a problem hiding this comment.
Because the python3 APIs we're implementing are unicode-aware:
https://docs.python.org/3/library/stdtypes.html#str.isdecimal
https://docs.python.org/3/library/stdtypes.html#str.isnumeric
Thankfully they're not locale-aware, that would be way worse.
There was a problem hiding this comment.
I guess in principle one could support it right? I'm totally fine not doing so, though!
|
It turns out to be a pypy bug - probably OK to just xfail the tests on pypy (unless @mattip would prefer something else?). |
|
Thanks for looking into this @ngoldbaum! I've xfail'd the tests. |
|
The two test failures seem unrelated now. |
|
Darn, not sure what's up with that sanitizers heisenbug. I hoped that was fixed by #25834 but apparently not. Restarted the failed test jobs. |
|
Passing now. @mhvk looked this over and approved, so let’s bring this one in. |
No description provided.