Repository navigation
ENH: Add expandtabs ufunc for string & unicode dtypes - #25891
Conversation
mhvk
left a comment
mhvk
left a comment
There was a problem hiding this comment.
Not sure why the xfail is not working, but the code itself looks good - just two tiny comments.
|
Hmm, |
| if (ch == '\n' || ch == '\r') { | ||
| line_pos = 0; | ||
| } | ||
| } |
There was a problem hiding this comment.
I guess it depends on what the input looks like, but I suspect for common real-world uses that instead of writing each non-tab character out individually, it would be faster to keep track of the index of the character after the last non-tab character and instead buffer_memcpy between there and here whenever we encounter a tab or the end of the string.
There was a problem hiding this comment.
With that, the code would become rather like replace, except with a more variable replacement string. Should be possible to basically copy the loop from there to implement the faster copying.
There was a problem hiding this comment.
Agreed, it'd most probably be faster. Let's merge this to get into NumPy 2.0 and I'll open a follow-up issue.
Oops, that was me being lazy and not worrying about the The real fix is just to implement this for stringdtype. Hoping to push an implementation to this PR this afternoon. Don't worry about the other comment I left about the algorithm if you're not interested in trying an implementation, the suggestion I left there is an optimization we could come back to later. |
| PyArrayMethodObject *NPY_UNUSED(self), | ||
| PyArray_DTypeMeta *NPY_UNUSED(dtypes[3]), | ||
| PyArray_Descr *given_descrs[3], | ||
| PyArray_Descr *loop_descrs[3], |
There was a problem hiding this comment.
FWIW I have these typed as e.g. loop_descrs[] in stringdtype so there's no need to make sure they're the correct length in the signature.
|
Another comment is that it would be nice to add tests with some unicode characters. The existing test you added didn't catch a bug in a WIP version of the last commit I just pushed with stringdtype that only showed up with some unicode characters in the string. |
3fb9820 to
53f5a6e
Compare
|
@ngoldbaum I added a couple of tests with unicode, but one fails with StringDType. I looked a tiny bit into it, but wasn't able to completely understand what's going on. |
|
Sorry about that! See the last commit, |
| if (new_len > PY_SSIZE_T_MAX - 1) { | ||
| npy_gil_error(PyExc_OverflowError, "new string is too long"); | ||
| return -1; | ||
| } |
There was a problem hiding this comment.
@ngoldbaum Wouldn't this overflow if we have a string where there's a tab right before reaching PY_SSIZE_T_MAX? This might lead to an incr of more than one, which means that new_len will turn negative. Am I missing something?
There was a problem hiding this comment.
Thanks for pointing that out! Is checking if new_len is equal to `Py_SSIZE_T_MAX or negative sufficient? I just pushed a fixup.
Constructing such a string on a 64 bit system isn't feasible so it's not straightforward to test this.
There was a problem hiding this comment.
That is okay, yes. Let's merge this!
|
@mhvk already looked this over and Lysandros reviewed the code I pushed afterwards, so I feel comfortable merging. I'll hit the button assuming all the tests pass. |
No description provided.