ENH: Implement string comparison ufuncs (or almost) #21716
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* ENH: Implement string comparison ufuncs (or almost) This makes all comparison operators and ufuncs work on strings using the ufunc machinery. It requires a half-manual "ufunc" to keep supporting void comparisons and especially `np.compare_chararrays` (that one may have a bit more overhead now). In general the new code should be much faster, and has a lot of easier optimization potential. It is also much simpler since it can outsource some complexities to the ufunc/iterator machinery. This further fixes a couple of bugs with byte-swapped strings. The backward compatibility related change is that using the normal ufunc machinery means that string comparisons between string and unicode now give a `FutureWarning` (instead of just False). * MAINT: Do not use C99 tagged struct init in C++ C++ does not like it (at least not before C++20)... GCC and clang don't seem to mind, but MSVC seems to. * BENCH: Add basic string comparison benchmarks * DOC,STY: Fixup string-comparisons comments based on review Thanks to Marten's comments, a few clarfications and slight fixups. * ENH: Use `memcmp` because it may be faster for the byte case * TST: Improve string and unicode comparison tests. * MAINT: Use switch statement based on review As suggested be Serge. Co-authored-by: Serge Guelton <serge.guelton@telecom-bretagne.eu> * TST: Make unicode byte-swap test slightly more concrete The issue is that the `view` needs to use native byte-order, so just ensure native byte-order for the view, and then do another cast to get it right. * BUG: Add `np.compare_chararrays` to test and fix typo * TST: Add test for empty string comparisons * TST: Fixup string test based on martens review * MAINT: Move definitions back into string_ufuncs.h * MAINT: Use enum class for comparison operator templating This removes the need for a dynamic (or static) assert in the switch statement. * Template version of add_loop to avoid redundant code * STY: Fixup style, two spaces, error is -1 * STY: Small `string_ufuncs.cpp` fixups based on Serge's review * MAINT: Fix merge conflict (ensure_dtype_nbo was removed) Co-authored-by: Serge Guelton <serge.guelton@telecom-bretagne.eu>
|
Oh, I never added a release note over in the original PR? Assuming this backport goes in, do you want me to add a release notes on this PR directly, or make a separate PR later? I have to check how much actually changed here, I may have been confusing the structured comparison PR with this one, and this one should have no changes except bug-fixes. But it is maybe still a nice new feature to note :). |
|
What/if you decide for the release note, just post it here and I will put it into the 1.23.0 release note in preparation for the rc3 release. |
|
I suppose could add a brief note under improvements? |
|
Thanks Sebastian, I've added that note. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Backport of #21041.
This makes all comparison operators and ufuncs work on strings
using the ufunc machinery.
It requires a half-manual "ufunc" to keep supporting void comparisons
and especially
np.compare_chararrays(that one may have a bit moreoverhead now).
In general the new code should be much faster, and has a lot of easier
optimization potential. It is also much simpler since it can outsource
some complexities to the ufunc/iterator machinery.
This further fixes a couple of bugs with byte-swapped strings.
The backward compatibility related change is that using the normal
ufunc machinery means that string comparisons between string and
unicode now give a
FutureWarning(instead of just False).C++ does not like it (at least not before C++20)... GCC and clang
don't seem to mind, but MSVC seems to.
BENCH: Add basic string comparison benchmarks
DOC,STY: Fixup string-comparisons comments based on review
Thanks to Marten's comments, a few clarfications and slight fixups.
ENH: Use
memcmpbecause it may be faster for the byte caseTST: Improve string and unicode comparison tests.
MAINT: Use switch statement based on review
As suggested be Serge.
Co-authored-by: Serge Guelton serge.guelton@telecom-bretagne.eu
The issue is that the
viewneeds to use native byte-order, sojust ensure native byte-order for the view, and then do another cast
to get it right.
BUG: Add
np.compare_chararraysto test and fix typoTST: Add test for empty string comparisons
TST: Fixup string test based on martens review
MAINT: Move definitions back into string_ufuncs.h
MAINT: Use enum class for comparison operator templating
This removes the need for a dynamic (or static) assert in the
switch statement.
Template version of add_loop to avoid redundant code
STY: Fixup style, two spaces, error is -1
STY: Small
string_ufuncs.cppfixups based on Serge's reviewMAINT: Fix merge conflict (ensure_dtype_nbo was removed)
Co-authored-by: Serge Guelton serge.guelton@telecom-bretagne.eu