ENH: fixes for StringDType sorting and comparisons for missing data - #32564
Conversation
710ba0a to
ac85d11
Compare
ac85d11 to
18f077d
Compare
|
@seberg any chance I can get you to look at this one as well? I just pushed a commit that cuts down the new tests quite a bit. Most of this is still new tests, the actual functional changes are small and I think the comparison fix is clearly correct. I have a nice followup for this that makes My fix relies entirely on StringDType's own hashing and sorting implementation. However making it work correctly required merging this and #32563. |
seberg
left a comment
There was a problem hiding this comment.
Thanks, are you sure about backporting? I am wondering if we are getting too aggressive, again; but fine if this is a real world issue. (Not even sure Chuck has generated release-notes for those in the past, but that can be done.)
The other thing I'll note is that (pd.NA != object()) is pd.NA, so for that this is all a bit unclear if it is defined at all.
That said, I guess it is still the better/typically expected behavior.
I do insist on that test, though!
| assert_array_equal(left == right, expected) | ||
| assert_array_equal(left != right, ~expected) | ||
| assert_array_equal(left == "", [True, False, False, False, True, False]) | ||
| assert_array_equal("" != left, [False, True, True, True, False, True]) |
There was a problem hiding this comment.
I'm sure it's correctly used in the comparison ufunc, but please expand this to all comparisons.
5f0c3d2 to
5fc53d7
Compare
PR summary
Fixes a couple bugs in the StringDType implementation, where missing data doesn't properly implement equality operators.
For nan-like missing data, it's the
!=operator:The answer for the last expression should be
array([False, True, True]). The current answer is a straight-up bug. Unfortunately it also changes operations likearr[data != sentinel], so we should probably not backport it.This also fixes this issue:
NumPy says
None == ""here! Under this PR, you get:As a result of these two fixes, also fixes issues with the
np.uniquecode paths that use the sort-based implementation, see the tests.This also includes a bugfix for numpy's testing utilities. At some point I added a second implementation of
StringDType's missing value classification, which is broken in a way that the tests added here expose. The fix is to defer to the rules the dtype actually uses.AI Disclosure
I iterated with an AI on this.