Conversation
jorenham
added
41 - Static typing
component: numpy.strings
|
ping @ngoldbaum in case you're interested |
| old: _CharLike_co, | ||
| new: _CharLike_co, |
There was a problem hiding this comment.
FWIW this is a runtime error:
>>> np.strings.replace("abc", np.bytes_("a"), np.bytes_("b"))
# UFuncTypeErrorThis works though, although maybe this is a bug?
>>> np.strings.replace("abc", b"a", b"b")
array('bbc', dtype='<U3')FWIW Python thinks this shouldn't work (IMO it's right):
>>> "abc".replace(b"a", b"b")
TypeError: replace() argument 1 must be str, not bytesBy the way, how do you want to handle NEP-58 and this PR? Would you like to help me expand this to include ByteStringDType? And maybe help catch edge cases like this?
We could also just declare that all of this weirdness with the np.str_ and np.bytes_ dtypes is legacy behavior that we're not going to change. ByteStringDType/StringDType already handle this. Unfortunately for users to see that, we'd need to change the coercion rules for str and bytes which is a big change.
There was a problem hiding this comment.
FWIW this is a runtime error:
>>> np.strings.replace("abc", np.bytes_("a"), np.bytes_("b")) # UFuncTypeErrorThis works though, although maybe this is a bug?
>>> np.strings.replace("abc", b"a", b"b") array('bbc', dtype='<U3')
np.bytes_ subclasses builtins.bytes, so there's no way to reject np.bytes_ when builtins.bytes is accepted, I'm afraid.
FWIW Python thinks this shouldn't work (IMO it's right):
>>> "abc".replace(b"a", b"b") TypeError: replace() argument 1 must be str, not bytes
If this isn't intended behavior, then we can just tighten this to only allow str here?
By the way, how do you want to handle NEP-58 and this PR?
That depends on the runtime behavior I guess.
Would you like to help me expand this to include ByteStringDType? And maybe help catch edge cases like this?
Sure :)
There was a problem hiding this comment.
If this isn't intended behavior, then we can just tighten this to only allow str here?
I'll look at fixing this problem as well.
There was a problem hiding this comment.
And just to be clear, are we talking about just replace, or the others as well ([r]partition, center, {r,l}just)?
There was a problem hiding this comment.
For strings.center there's an issue with that I see, because the fillchar default is a hard-coded " ":
Line 693 in 9ef82ea
There was a problem hiding this comment.
I'll be opening a PR for all of np.strings shortly. Unfortunately IMO it needs a deprecation too.
There was a problem hiding this comment.
For strings.center there's an issue with that I see, because the fillchar default is a hard-coded " "
IMO we should change those defaults to np._NoValue. I don't think there are any compatibility issues with that.
There was a problem hiding this comment.
Alright I'll wait a bit then
fca9295 to
c35bd32
Compare
|
blocker: #32757 |
This branch has not been deployed
2 participants
This adds shape-typing support to all public
numpy.strings.*functions when the input is an array with known shape-type, or for <=2d array-likes (nested sequences of str or bytes).See the commit messages for the full list the individual functions (so let's not squash merge this).
(I tried using stacked PRs, but apparently that doens't work from a fork, and I didn't want to push 15 branches to the upstream numpy repo.)
Pair programmed with AI (Fable 5.1 + ChatGPT Astra 6 as adverserial reviewer).