Repository navigation
BUG: fix StringDType coerce flag in binary ufunc promotion (#31861) - #31862
Conversation
shipitdev
left a comment
shipitdev
left a comment
There was a problem hiding this comment.
Thanks for the patch, @Ethan0456!
For a maintainer reviewing this (just laying out the PR simply for an easier merge):
I reviewed this, and it looks like whoever originally coded this just mistyped int out_coerce = descr1->coerce && descr1->coerce; when trying to resolve the output coerce flag. Because it checked descr1 twice and ignored descr2, binary_resolve_descriptors was returning the wrong output descriptor if the two arrays had different coerce flags.
The tests correctly isolate the bug, seem totally fine to me. Overall, this looks like a super easy merge to me! Thank you.
ngoldbaum
added
00 - Bug
09 - Backport-Candidate
| "coerce_a, coerce_b", | ||
| [(True, False), (False, True)], | ||
| ) | ||
| def test_add_preserves_coerce_false(coerce_a, coerce_b): |
There was a problem hiding this comment.
Instead of adding a new test, couldn't you simply add an assert to test_unset_na_coercion:
numpy/numpy/_core/tests/test_stringdtype.py
Lines 1813 to 1826 in b8853b2
Something like:
expected_coerce = op_dtype is None or op_dtype.coerce
assert res.dtype == StringDType(na_object=None, coerce=expected_coerce)
There was a problem hiding this comment.
Good catch. I have dropped the new test and extended test_unset_na_coercion; now each branch builds the expected result dtype (including coerce) before asserting it.
There was a problem hiding this comment.
Thanks for catching this, I agree the fix is correct.
I left one comment about the new test, which I think is duplicative of existing tests.
AIs like to write new tests, it's good to prompt them to prefer expanding existing tests rather than to write new tests.
|
@ngoldbaum Addressed your review feedback in the latest commit, thanks! |
|
Thanks @Ethan0456! |
BUG: fix `StringDType` coerce flag in binary ufunc promotion (#31862)
…1861) (numpy#31862) (cherry picked from commit 5075498)
PR summary
Fixes #31861.
Binary
StringDTypeufuncs resolved outputcoercewithdescr1->coerce && descr1->coerceinstead of combining both operands.That made results order-dependent (e.g.
np.addwithcoerce=Truethencoerce=Falsekeptcoerce=True) and allowed non-string assignment via implicit stringification.This changes the combination to
descr1->coerce && descr2->coerce, matchingcommon_instance/np.promote_types, and adds a small regression test for both operand orders.First time committer introduction
I have been using NumPy for some time now. I hit this while working with
StringDTypeandcoerce, and found two places with similar typo bug. I checked and found that one typo incommon_instancehas been fixed yesterday, so wanted to fix the remaining typo in the binary ufunc path.AI Disclosure
I used Grok to help locate the bug in
binary_resolve_descriptors, draft the regression test, and refine the issue/PR wording. I made and verified the fix with correctness checks, rebuilds and the test suite; final review and PR submission were done by me.