Conversation
ngoldbaum
added
07 - Deprecation
component: numpy.strings
There was a problem hiding this comment.
Two comments. I'd love to fix this one level lower, but since I am not sure how viable it is and strings need some annoying extra handling anyway (e.g. to deal with the default), I am not opposed to this.
It does seem like there is absolutely no reason not to use None as default, though?
| "The fill character must be exactly one character long") | ||
| fillchar = _get_fillchar(a, fillchar) | ||
|
|
||
| if np.result_type(a, fillchar).char == "T": |
There was a problem hiding this comment.
Maybe it doesn't matter, but as a note. There is an underlying issue here that:
np.result_type() happily promotes stringes/bytes and everything else to strings. I would love to fix this, but IIRC there was some fun fallout (that may be fixable, IIRC a main thing was that some code relied on it working for later things to not fail).
That means that np.concatenate(([b"1", "2"])) just as happily works for example.
(I think that can_cast("U1", "S1", casting="safe") is true is probably even fine.)
Bringing this up not to stop it, but to say that there is more to this and I suppose I semi-care about this if we don't attack the rest. It might even be interesting to start with result_type(..., strict_strings=True) even if private.
There was a problem hiding this comment.
It might even be interesting to start with result_type(..., strict_strings=True) even if private.
I went ahead and did that because I think it's a cleaner fix. I'm also about to file a followup issue citing your comment about this.
| @set_module("numpy.strings") | ||
| @array_function_dispatch(_just_dispatcher) | ||
| def center(a, width, fillchar=' '): | ||
| def center(a, width, fillchar=np._NoValue): |
There was a problem hiding this comment.
I really don't see why you use np._NoValue, when None should do, I think? The only reason I can see would be that None currently actually places the string "None"
There was a problem hiding this comment.
I'm guessing it's because StringDType and upcoming ByteStringDType could use None as na_object?
There was a problem hiding this comment.
None works! Even if it comes from missing data, it still means "use a space". That's not inconsistent. Also it's a super corner case.
There was a problem hiding this comment.
Ok nice, None is a bit cleaner in the stubs as well because then we can fill in the = ...
Co-authored-by: Joren Hammudoglu <[email protected]>
41a0d05 to
52514f5
Compare
|
I ended up applying part of the deeper structural solution @seberg was thinking about. It does make the diff bigger though, let me know what you think. I'll file a followup issue about the array coercion issues that Sebastian pointed out. |
This branch has not been deployed
3 participants
PR summary
Over in https://github.com/numpy/numpy/pull/32749/changes#r4072990536 @jorenham and I identified an inconsistency with how python handles mixed string and bytes operands in string and bytes methods.
First, this changes a number of default arguments in
np.stringsfrom empty string toNone. This allows us to properly handle bytes operands for these functions and also avoid emitting warnings when the argument is not explicitly passed. To avoid breaking possible downstream__array_function__implementations, I only pass these arguments if a user sets them, so downstream can continue assuming the argument is always a string.Second, it deprecates calls like
np.strings.replace(b"abc", "a", "x")that silently mix text and bytes. This is deprecated forreplace,partition, and text-padding functions. My AI model found concrete downstream sites in where this pattern happens:S4, then callsnp.char.ljust(..., fillchar=" ").np.char.replace(objects['NAME'], '_', ' ')on anS24field.It also adds machinery in
_array_converterto facilitate these checks.Finally, updates the type stubs to match these changes.
AI Disclosure
I iterated on this using an AI model.