FIX-#6287: Fix handling of numpy array_function #6288
Conversation
Signed-off-by: Rehan Durrani <[email protected]>
Signed-off-by: Rehan Durrani <[email protected]>
Signed-off-by: Rehan Durrani <[email protected]>
Signed-off-by: Rehan Durrani <[email protected]>
|
@RehanSD CI is broken |
Signed-off-by: Rehan Durrani <[email protected]>
Signed-off-by: Rehan Durrani <[email protected]>
| @@ -3426,6 +3426,80 @@ def __and__(self, other): | |||
| def __rand__(self, other): | |||
| return self._binary_op("__rand__", other, axis=0) | |||
|
|
|||
| def __array_function__(self, func, types, args, kwargs): | |||
There was a problem hiding this comment.
both array_ufunc and array_function will not work if the user passes in a Modin NumPy Array for out if we end up defaulting to NumPy. In the Modin NumPy API, I've fixed it so that we do copy in the values to a Modin NumPy Array, but I'm not sure what the solution should be for this, and so have defaulted to having it be unsupported.
There was a problem hiding this comment.
I don't see where this "unsupported" is placed in code. I think it should be an exception or something similar, shouldn't it?
Signed-off-by: Rehan Durrani <[email protected]>
Signed-off-by: Rehan Durrani <[email protected]>
| # If `out` is a modin.numpy.array, `kwargs.get("out")` returns a 1-tuple | ||
| # whose only element is that array, so we need to unwrap it from the tuple. | ||
| out_kwarg = out_kwarg[0] | ||
| kwargs.pop("out", None) |
There was a problem hiding this comment.
I think we should merge the .get and .pop like
out_kwarg = kwargs.pop("out", None)I see no point of keeping them separated
| if where_kwarg is not None: | ||
| if hasattr(where_kwarg, "_query_compiler"): | ||
| kwargs["where"] = where_kwarg.__array__() | ||
| return func(arr, *converted_args[1:], **kwargs) |
There was a problem hiding this comment.
this seems to be losing converteg_args[0] (which was args[0]), feels weird
There was a problem hiding this comment.
I can add a comment - but basically args[0] is self. Since we’re calling the function directly instead of calling the special method again, I’m passing in the converted self as the first argument, and only need to pass in any remaining args (after making sure to convert them)
There was a problem hiding this comment.
a comment would be nice; also, if we're dropping converted_args[0], then we should probably be doing so in the upper loop and skip the call of args[0].__array__() altogether
Co-authored-by: Vasily Litvinov <[email protected]>
| @@ -397,10 +397,29 @@ def __array_ufunc__(self, ufunc, method, *inputs, **kwargs): | |||
| if isinstance(input, pd.Series): | |||
| input = input._query_compiler.to_numpy().flatten() | |||
| args += [input] | |||
| out_kwarg = kwargs.get("out", None) | |||
There was a problem hiding this comment.
| out_kwarg = kwargs.get("out", None) | |
| out_kwarg = kwargs.pop("out", None) |
| # If `out` is a modin.numpy.array, `kwargs.get("out")` returns a 1-tuple | ||
| # whose only element is that array, so we need to unwrap it from the tuple. | ||
| out_kwarg = out_kwarg[0] | ||
| kwargs.pop("out", None) |
There was a problem hiding this comment.
| kwargs.pop("out", None) |
| @@ -3426,6 +3426,80 @@ def __and__(self, other): | |||
| def __rand__(self, other): | |||
| return self._binary_op("__rand__", other, axis=0) | |||
|
|
|||
| def __array_function__(self, func, types, args, kwargs): | |||
| if where_kwarg is not None: | ||
| if hasattr(where_kwarg, "_query_compiler"): | ||
| kwargs["where"] = where_kwarg.__array__() | ||
| return func(arr, *converted_args[1:], **kwargs) |
There was a problem hiding this comment.
a comment would be nice; also, if we're dropping converted_args[0], then we should probably be doing so in the upper loop and skip the call of args[0].__array__() altogether
What do these changes do?
flake8 modin/ asv_bench/benchmarks scripts/doc_checker.pyblack --check modin/ asv_bench/benchmarks scripts/doc_checker.pygit commit -sdocs/development/architecture.rstis up-to-date