Skip to content

Implement __array__ protocol #87

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.

Sign up for GitHub

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

Jump to bottom
Merged
mrocklin merged 2 commits into from
Jan 25, 2018
Merged

Implement __array__ protocol #87

mrocklin merged 2 commits into from
Jan 25, 2018

Conversation

@mrocklin
Copy link
Contributor

@mrocklin mrocklin commented Jan 25, 2018

One approach to the problems described in #72

@mrocklin mrocklin mentioned this pull request Jan 25, 2018
Closed
@mrocklin
Copy link
Contributor Author

mrocklin commented Jan 25, 2018

I haven't thought through this very deeply. Criticism welcome from @nils-werner and @hameerabbasi

hameerabbasi
hameerabbasi reviewed Jan 25, 2018
View reviewed changes
sparse/tests/test_coo.py Outdated

assert_eq(x, xx)
assert_eq(x.T.dot(x), xx.T.dot(xx))
assert isinstance(x + xx, (COO, scipy.sparse.spmatrix))
Copy link
Collaborator

@hameerabbasi hameerabbasi Jan 25, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps test only for COO? Since we're "higher level" than scipy.sparse, it makes sense that mixed binary operations don't return scipy.sparse.spmatrix. Otherwise we have things like x + xx + x returning scipy.sparse.spmatrix.

Copy link
Collaborator

@hameerabbasi hameerabbasi Jan 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In addition (just as a minor quip) I would parametrize this with format for scipy.sparse and check for (at least) csr, csc, coo and dok.

Copy link
Contributor Author

@mrocklin mrocklin Jan 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

hameerabbasi
hameerabbasi reviewed Jan 25, 2018
View reviewed changes
sparse/coo.py Outdated
x = self.todense()
if dtype and x.dtype != dtype:
x = x.astype(dtype)
if not isinstance(x, np.ndarray):
Copy link
Collaborator

@hameerabbasi hameerabbasi Jan 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe np.asarray? It passes through things that are already arrays.

But, in any case, do we really need this check, since .todense() already returns an np.ndarray.

Copy link
Contributor Author

@mrocklin mrocklin Jan 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed

hameerabbasi
hameerabbasi requested changes Jan 25, 2018
View reviewed changes
Copy link
Collaborator

@hameerabbasi hameerabbasi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments. It seems to work fine! spmatrix + COO returns COO as desired now! (at least with CSR, CSC and COO).

In addition, for now it's okay, but later with #10 (which I'm working on next), I would like to treat this with the auto-densification rules...

@hameerabbasi
Copy link
Collaborator

hameerabbasi commented Jan 25, 2018 •
edited
Loading

Hmm. I just tested this some more:

>>> x * xx
Traceback (most recent call last):
  File "/Users/hameerabbasi/anaconda/lib/python3.6/site-packages/IPython/core/interactiveshell.py", line 2910, in run_code
    exec(code_obj, self.user_global_ns, self.user_ns)
  File "<ipython-input-7-d2266dae7fac>", line 1, in <module>
>>> x * xx
  File "/Users/hameerabbasi/anaconda/lib/python3.6/site-packages/scipy/sparse/base.py", line 476, in __mul__
    raise ValueError('dimension mismatch')
ValueError: dimension mismatch
>>> xx * x
Out[8]: <COO: shape=(100, 10), dtype=uint8, nnz=96, sorted=False, duplicates=False>

@mrocklin
Copy link
Contributor Author

mrocklin commented Jan 25, 2018

Can you explain more about how you're getting that error? I'm not able to reproduce.

@hameerabbasi
Copy link
Collaborator

hameerabbasi commented Jan 25, 2018

add and sub work fine, mul gives that error.

@mrocklin
Copy link
Contributor Author

mrocklin commented Jan 25, 2018

Yeah, I mentioned this in the other PR, but just for completeness __mul__ in scipy.sparse matrices will be matrix-multiplication, which I expect would be a bit stranger. Personally I'm ok seeing this fail.

@hameerabbasi
Copy link
Collaborator

hameerabbasi commented Jan 25, 2018

In that case, this gets a +1 from me. :)

@hameerabbasi
Copy link
Collaborator

hameerabbasi commented Jan 25, 2018

We should merge this, I'll rebase #84 on top of it, and then I'm totally fine releasing.

@mrocklin mrocklin merged commit 5021571 into master Jan 25, 2018
@mrocklin mrocklin deleted the array-protocol branch January 25, 2018 13:23
@mrocklin
Copy link
Contributor Author

mrocklin commented Jan 25, 2018

@hameerabbasi ok to release current master as 0.2.0 ?

@hameerabbasi
Copy link
Collaborator

hameerabbasi commented Jan 25, 2018

Of course. :)

hameerabbasi pushed a commit to hameerabbasi/sparse that referenced this pull request Feb 27, 2018
@mrocklin @hameerabbasi
Implement __array__ protocol (pydata#87)
5a23f6e 
* Implement __array__ protocol

* add parametrized test for scipy sparse interactions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@hameerabbasi hameerabbasi hameerabbasi approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mrocklin @hameerabbasi