Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support left-side np.number elemwise operations. #67

Merged
merged 5 commits into from Jan 8, 2018

Conversation

fujiisoup
Copy link
Member

Fixes #66.

This adds a workaround when the np.number multiplication from left-side.

@hameerabbasi
Copy link
Collaborator

Hello, the correct way to fix this would be to modify __rmul__ to return scalar * COO. Fixing it this way causes the broadcasting path to be taken which will be much, much slower and will fail for scalar COO objects, returning 1-D instead of scalar.

@fujiisoup
Copy link
Member Author

Hi, @hameerabbasi
Thanks for the quick response.

the correct way to fix this would be to modify rmul to return scalar * COO

np.number has __mul__ method and thus COO.__rmul__ will not be called.
Instead, np.number.__mul__ calls COO.__array_ufunc__.

Another option would be to have a special treatment for the func is operator.mul case in __array_ufunc__, but the code would be more complicated.

@hameerabbasi
Copy link
Collaborator

hameerabbasi commented Jan 6, 2018

Hey! I came up with a long-term solution to this, hope it will be of use.

@staticmethod
    def _elemwise(func, *args, **kwargs):
        assert len(args) >= 1
        self = args[0]
        if isinstance(self, scipy.sparse.spmatrix):
            self = COO.from_numpy(self)
        elif np.isscalar(self):
            func = partial(func, self)
            other = args[1]
            if isinstance(other, scipy.sparse.spmatrix):
                other = COO.from_scipy_sparse(other)

            return other._elemwise_unary(func, *args[2:], **kwargs)

        if len(args) == 1:
            return self._elemwise_unary(func, *args[1:], **kwargs)
        else:
            other = args[1]
            if isinstance(other, scipy.sparse.spmatrix):
                other = COO.from_scipy_sparse(other)

            if isinstance(other, COO):
                return self._elemwise_binary(func, other, *args[2:], **kwargs)
            else:
                return self._elemwise_unary(func, *args[1:], **kwargs)

If you check this in, maybe @mrocklin can review it.

Edit: You need to import functools.partial.

Edit 2: Edited to account for unit test failure.

@hameerabbasi
Copy link
Collaborator

@fujiisoup I've edited the elemwise to account for the unit test failure.

@fujiisoup
Copy link
Member Author

Cool!!

I added more tests for left-side operations.
Additionally, I added a support also for 0-dimensional np.ndarray.

@hameerabbasi
Copy link
Collaborator

Would you be kind enough to rebase this on master? Your other PR caused conflicts with this one.

# Conflicts:
#	sparse/tests/test_core.py
@hameerabbasi
Copy link
Collaborator

Just waiting on either of @nils-werner or @mrocklin to review these changes... Since I was involved in adding code, I shouldn't be the one to review it. :-)

@hameerabbasi hameerabbasi changed the title Support left-side np.number multiplication Support left-side np.number elemwise operations. Jan 6, 2018
@hameerabbasi hameerabbasi merged commit 49f6479 into pydata:master Jan 8, 2018
@mrocklin
Copy link
Collaborator

mrocklin commented Jan 8, 2018

Thank you for the fix @fujiisoup . I'm sorry I wasn't around to review (I'm in conference prep mode). Thank you for handling this @hameerabbasi .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants