ENH: add NEP-50-style string promotion outside ufuncs - #32356
Conversation
Sorry, something went wrong.
mhvk
left a comment
There was a problem hiding this comment.
Looks good! Most comments just nits about organization.
p.s. There's part of me that still wonders whether str shouldn't be treated more just like the numerical scalars, but I guess we can always change that if needed.
Sorry, something went wrong.
| Py_DECREF(item); | ||
| } | ||
|
|
||
| /* Explicit axes reject the 0-d str before resolving a descriptor */ |
There was a problem hiding this comment.
This can be inside if (axis == NPY_RAVEL_AXIS) just below
Sorry, something went wrong.
| } | ||
|
|
||
| if (axis == NPY_RAVEL_AXIS) { | ||
| ret = PyArray_ConcatenateFlattenedArrays( |
There was a problem hiding this comment.
I guess for numerical scalers, the check is done inside ConcatenateFlattenedArrays -- it would seem more logical to do the same for str as well (if for some reason that's just unhandy, maybe add a comment here that the same checks for numerical scalars are done in ConcatenateFlattenedArrays).
Sorry, something went wrong.
| /* 'narrays' was set to how far we got in the conversion */ | ||
| for (iarrays = 0; iarrays < narrays; ++iarrays) { | ||
| Py_DECREF(arrays[iarrays]); | ||
| Py_XDECREF(arrays[iarrays]); |
There was a problem hiding this comment.
Why is this change necessary?
Sorry, something went wrong.
There was a problem hiding this comment.
I rewrote the PR so changing this isn't necessary anymore
Sorry, something went wrong.
| dst = np.empty(2, dtype=dtype) | ||
| np.copyto(dst, scalar) | ||
| assert dst[0] == scalar | ||
| np.copyto(dst, scalar, casting="unsafe") |
There was a problem hiding this comment.
I think you need to recreate the array to be sure the tests checks that copyto actually worked (since there was a copy already). Or is the casting="unsafe" needed if the array is already filled? If so, do a quick pytest.raises before with regular casting (and add a comment!).
Sorry, something went wrong.
|
|
||
| res = np.concatenate((arr, scalar), axis=None) | ||
| assert res.dtype == dtype | ||
| assert_array_equal(res, np.array(["y", scalar], dtype=dtype)) |
There was a problem hiding this comment.
If you add strict=True, you do not really need to assert the dtype above (though perhaps you prefer it for clarity).
Sorry, something went wrong.
|
@mhvk thanks for the suggestions! I think I've addressed your comments, mostly to reorganize and delete code that you were commenting on. This is a lot simpler now. I'd appreciate it if you could give it another look. |
Sorry, something went wrong.
mhvk
left a comment
There was a problem hiding this comment.
@ngoldbaum - it has indeed become a nice surgical addition! One remaining nitpick, but also a question about casting that struck me in the tests. I may just be missing something obvious, though!
Sorry, something went wrong.
|
|
||
| for (iarrays = 0; iarrays < narrays; ++iarrays) { | ||
| Py_DECREF(arrays[iarrays]); | ||
| Py_XDECREF(arrays[iarrays]); |
There was a problem hiding this comment.
I thought I saw come by that this change was no longer needed. Did you forget to push the change? Or was that in another piece of code. Anyway, no big deal if it is needed, but if not, maybe nice to undo.
Sorry, something went wrong.
|
|
||
| # A Python str adopts a StringDType's semantics in the same way | ||
| np.copyto(np.empty(3, dtype=np.dtypes.StringDType()), "x", casting="no") | ||
| with pytest.raises(TypeError): |
There was a problem hiding this comment.
I realize I'm actually confused: how can "no" casting be OK, but "equiv" not? I understand the ones above, where one goes up in stringency, from "safe" to "equiv", but here you go down!
Sorry, something went wrong.
PR summary
Followup for #32040
This adds NEP-50 style promotion for strings to copyto and where, which have explicit handling for numeric scalars. It also adds new promotion handling for concatenate and choose. These weren't needed before in concatenate and choose because numeric scalars don't need to do anything the the scalar value, only type metadata. Concatenate and choose correctly propagate that metadata but don't correctly propagate trailing NULLs.
The net effect is that trailing NULLs are now preserved for StringDType in all four operations, see tests.
AI Disclosure
I iterated on this with an AI model.