ENH: __array_function__ for np.core.defchararray
#12154
Conversation
| return (a,) | ||
|
|
||
|
|
||
| @array_function_dispatch(_partition_dispatcher) |
There was a problem hiding this comment.
I wonder if it makes sense to use lambda in these simple cases
There was a problem hiding this comment.
Yeah... I think there's a strong case for saying that it if fits on a single line with lambda, use that instead of a separate dispatcher (see my latest commit).
The downside is that it can look a little dense when there are a few arguments, e.g.,
@array_function_dispatch(lambda a, old, new, count=None: (a,))
def replace(a, old, new, count=None):
...There was a problem hiding this comment.
@mhvk @hameerabbasi any opinions on the style here?
There was a problem hiding this comment.
Actually, let's move discussion to the master issue.
There was a problem hiding this comment.
Could line wrap weirdly as:
@array_function_dispatch(lambda
a, old, new, count=None: (a,))
def replace(a, old, new, count=None):
...
There was a problem hiding this comment.
I do like this wrapped version (added to the example in the master issue):
@array_function_dispatch(
lambda a, old, new, count=None: (a,)
)
def replace(a, old, new, count=None):
...|
What is CLN as a prefix? |
|
I've been using "CLN" for "cleanup"... but maybe that's not in our official list? :) |
| @@ -377,6 +399,11 @@ def capitalize(a): | |||
| return _vec_string(a_arr, a_arr.dtype, 'capitalize') | |||
|
|
|||
|
|
|||
| def _center_dispatcher(a, width, fillchar=None): | |||
| return (a,) | |||
There was a problem hiding this comment.
It appears in the call in line 422. Do you mean not tested?
There was a problem hiding this comment.
I reverted my later commit that made this unused. So now it definitely is used.
|
I don't actually feel strongly about the lambdas thing, but it would be nice to be consistent with some of the already merged PRs. If you think it's better as lambdas, then perhaps we should revisit the existing non-lambda usage. Perhaps that should be discussed on the tracking issue? |
|
Azure missing from the CI list -- not sure why -- I'll open an issue |
|
Yep, CLN doesn't appear on our list - I think MAINT would be closest. |
dcc079b to
39fb169
Compare
|
I decided to revert the commits switching to use |
__array_function__ for np.core.defchararray
xref #12028