BUG: Re-enable overriding functions in the np.strings module.#28741
BUG: Re-enable overriding functions in the np.strings module.#28741ngoldbaum merged 8 commits intonumpy:mainfrom
np.strings module.#28741Conversation
…swapcase`, `capitalize`, `title`, `_join`, `_split`, `_rsplit`, `_splitlines`, and `translate` functions in the `np.strings` module.
mhvk
left a comment
There was a problem hiding this comment.
This looks great! Ideally, though, there are also tests to ensure we don't regress... I think it can be fairly simple, along the lines of
class TestOverride:
@classmethod
def setup_class(cls):
class Override:
def __array_function__(self, *args, **kwargs):
return "function"
def __array_ufunc__(self, *args, **kwargs):
return "ufunc"
cls.override = Override()
@pytest.mark.parametrize("func, kwargs", [
(np.strings.upper, {}),
(...),
])
def test_override_function(self, func, kwargs):
assert func(self.override, **kwargs) == "function"
... same for any ufuncs ...
Also, I think this warrants a changelog entry...
|
@mhvk, thanks for your review. I've tried to implement your suggestions, but some of the |
mhvk
left a comment
There was a problem hiding this comment.
@byrdie - the problem is that some of the functions pass the input through np.asanyarray - so the Override class gets turned into an ndarray with object dtype. I fear those functions just need an array_function_dispatch as well (and then move their test to the one using __array_function__).
Slightly annoying, but also good, in that the tests showed the PR was not quite complete yet -- you need 8 more function:
center
expandtabs
ljust
multiply
partition
replace
rjust
rpartition
zfill
|
I'm afraid I don't think I understand why the remaining tests are failing, they don't seem directly related to what I changed. |
mhvk
left a comment
There was a problem hiding this comment.
Thanks, this looks all OK now! The test failures indeed seem unrelated, so approving.
Might be good, though, for @ngoldbaum to have a quick look too.
mod, encode, decode, upper, lower, swapcase, capitalize, title, _join, _split, _rsplit, _splitlines, and translate functions in the np.strings module.np.strings module.
|
Please fix the documentation failure before merging. |
mhvk
left a comment
There was a problem hiding this comment.
Great that everything now passes too. I'll re-approve but will give it a few more days just in case @mattip has other comments, or @ngoldbaum wants to have a look. (If nothing happens, feel free to ping me middle of next week, and I'll merge.)
|
Thanks so much for working on this @byrdie! I'd still like to add a test that verifies the entire public API is overridable via either |
Addresses Issue #28732