Add @ operator (simplify) #16
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
added 9 commits
May 2, 2017 08:49
…al code, not NumPy's
Closed
jakevdp
reviewed
May 3, 2017
sparse/tests/test_core.py
Outdated
| la = a.tolist() | ||
| lb = b.tolist() | ||
| la, lb # silencing flake8 | ||
|
|
There was a problem hiding this comment.
These are not used anymore, I think
sparse/tests/test_core.py
Outdated
| sa = COO.from_numpy(a) | ||
| sb = COO.from_numpy(b) | ||
|
|
||
| assert_eq(a.dot(b), sa.dot(sb)) | ||
| assert_eq(np.dot(a, b), sparse.dot(sa, sb)) | ||
|
|
||
| if sys.version_info >= (3, 5): |
There was a problem hiding this comment.
Rather than an explicit version check, perhaps we could check hasattr(operator, 'matmul')
| assert_eq(eval("sa @ sb"), sparse.dot(sa, sb)) | ||
|
|
||
| # Test that SOO's and np.array's combine correctly | ||
| assert_eq(eval("a @ sb"), eval("sa @ b")) |
There was a problem hiding this comment.
I think we could also check np.dot(sa, sb). If the classes are set up correctly, this should know how to delegate to sparse.dot (though I think it requires __numpy_ufunc__)
There was a problem hiding this comment.
This gets a failure that doesn't seem straightforward to fix. But moreover, I wouldn't really expect it to work as a user.
sparse/core.py:448: in __mul__
return self.elemwise_binary(operator.mul, other)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
self = <COO: shape=(3, 4, 5), dtype=float64, nnz=6>
func = <built-in function mul>
other = <COO: shape=(5, 6), dtype=float64, nnz=4>, args = (), kwargs = {}
def elemwise_binary(self, func, other, *args, **kwargs):
assert isinstance(other, COO)
if kwargs.pop('check', True) and func(0, 0, *args, **kwargs) != 0:
raise ValueError("Performing this operation would produce "
"a dense result: %s" % str(func))
if self.shape != other.shape:
> raise NotImplementedError("Broadcasting is not supported")
E NotImplementedError: Broadcasting is not supported
sparse/core.py:478: NotImplementedErrorThere was a problem hiding this comment.
Weird.. that error comes from the fact that np.dot(sa, sb) ends up calling sa.__mul__(sb), which... I have no idea why it would do that.
jakevdp
reviewed
May 3, 2017
sparse/core.py
Outdated
| @@ -681,6 +686,8 @@ def tensordot(a, b, axes=2): | |||
|
|
|||
|
|
|||
| def dot(a, b): | |||
| if not hasattr(a, 'ndim') or not hasattr(b, 'ndim'): | |||
| return NotImplemented | |||
There was a problem hiding this comment.
I think this should be raise NotImplementedError, because dot is often called on its own. You could catch the NotImplementedError and return NotImplemented within __matmul__ if you want to use it.
|
I've pushed out the changes in response to review. |
Closed
|
I think this is good to merge, but first it needs to be rebased against master, and I also think the commit history should be cleaned up (though I'm not sure of @mrocklin's philosophy on that...) |
|
Clean commit history or just squashing would be fine. Honestly though I
haven't taken great care with this project.
…On Mon, May 8, 2017 at 1:00 PM, Jake Vanderplas ***@***.***> wrote:
I think this is good to merge, but first it needs to be rebased against
master, and I also think the commit history should be cleaned up (though
I'm not sure of @mrocklin <https://github.com/mrocklin>'s philosophy on
that...)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<https://github.com/mrocklin/sparse/pull/16#issuecomment-299926331>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AASszCE4SoM3iwsuToouB9aAE_-wmxYnks5r30pFgaJpZM4NP-KH>
.
|
added 8 commits
May 9, 2017 09:21
…al code, not NumPy's
|
I'm lacking some git-fu to cleanup merge conflicts in rebasing and also prune commit history. Would it be OK/reasonable if I simply open a new PR using a different branch of my fork (where hopefully I can clean the commit history)? |
|
Whatever works for you. I apologize for being absent in this PR. Normal
work duties are taking precedence. I'll try to take a look at this tonight.
…On Tue, May 9, 2017 at 1:29 PM, David Mertz ***@***.***> wrote:
I'm lacking some git-fu to cleanup merge conflicts in rebasing and also
prune commit history. Would it be OK/reasonable if I simply open a new PR
using a different branch of my fork (where hopefully I can clean the commit
history)?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<https://github.com/mrocklin/sparse/pull/16#issuecomment-300241265>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/AASszBTtQH-rLObDOJQ0eUWuwl_QFgmYks5r4KJcgaJpZM4NP-KH>
.
|
added 3 commits
May 9, 2017 11:56
# This is the 1st commit message: # This is a combination of 3 commits. # This is the 1st commit message: # This is a combination of 3 commits. # This is the 1st commit message: # This is a combination of 4 commits. # This is the 1st commit message: Add @ operator (__matmul__ and __rmatmul__) and tests. Do not implem ent coersion of Python lists/tuples. # This is the commit message pydata#2: test for __rmatmul__ # This is the commit message pydata#3: Better implementation of __rmatmul__ and test that exercises the actual code, not NumPy's # The commit message pydata#4 will be skipped: # Cleanup tests of coersion failure # The commit message pydata#2 will be skipped: # # This is a combination of 2 commits. # # This is the 1st commit message: # # Resolve merge conflict in rebasing # # # The commit message pydata#2 will be skipped: # # # test for __rmatmul__ # The commit message pydata#3 will be skipped: # Resolve merge conflict in rebasing # The commit message pydata#2 will be skipped: # test for __rmatmul__ # The commit message pydata#3 will be skipped: # Better implementation of __rmatmul__ and test that exercises the actual code, not NumPy's # This is the commit message pydata#2: Remove handling of lists etc. # The commit message pydata#3 will be skipped: # Remove handling of lists etc.
added 5 commits
May 9, 2017 12:03
# This is the 1st commit message: # This is a combination of 2 commits. # This is the 1st commit message: # This is a combination of 2 commits. # This is the 1st commit message: Resolve merge on rebase # The commit message pydata#2 will be skipped: # Resolve merge on rebase # The commit message pydata#2 will be skipped: # Better implementation of __rmatmul__ and test that exercises the actual code, not NumPy's # The commit message pydata#2 will be skipped: # Remove handling of lists etc. # The commit message pydata#3 will be skipped: # Remove handling of lists etc.
|
Merged – thanks! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This supersedes the closed https://github.com/mrocklin/sparse/pull/14.
This request simply adds the
__matmul__and__rmatmul__with no behavior other than that delegated tosparse.dot(). Checks for "correct" non-combination of lists with COOs remains. A more specificreturn NotImplementedis returned for things lacking an.ndim, which gives us a better error of: