8000 Matmul/generic2 by DavidMertz · Pull Request #18 · pydata/sparse · GitHub
[go: up one dir, main page]

Skip to content

Matmul/generic2 #18

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

Closed
wants to merge 8 commits into from
Closed

Conversation

DavidMertz
Copy link

This is an alternative to the approach of https://github.com/mrocklin/sparse/pull/17/files. No more than one of them should be accepted.

The gist of this is that—in keeping with NumPy and scipy.sparse behavior—subclasses of list or tuple will be automatically converted to NumPy (dense) arrays if they are passed as an argument to sparse.dot(). Any other type of object lacking an .ndim will simply cause an exception to be raised.

In contrast to PR 17, this allows no opt-out or opt-in from the coercion behavior. However, since the initial thing coerced is always a (dense) list/tuple, it cannot be too big to represent in memory (and the np.array version will only be smaller, albeit an additional object in memory).

@DavidMertz DavidMertz mentioned this pull request May 3, 2017
@jakevdp
Copy link
Contributor
jakevdp commented May 3, 2017

I think this can be solved much more generally by using np.asanyarray after checking for either explicitly-supported types or checking for ducktypes. You'd then have to check that the result of np.asanyarray has an appropriate dtype and shape, and if not, raise an appropriate error. That would allow this to be used with any Python object that exposes the buffer interface. That's the solution used by scipy.sparse.

The downside is that this would not allow people to define other kinds of sparse objects that would work with COO matrices... we could solve that by specifying an __array_priority__ of, say, "10.2", so it would be just above that of scipy.sparse. Then someone wanting to extend this behavior could set an even higher priority if they want to control dot interactions with their own objects.

@jakevdp
Copy link
Contributor
jakevdp commented May 3, 2017

But __array_priority__ may be superseded by __numpy_ufunc__, though I'm not sure...

@mrocklin
Copy link
Contributor
mrocklin commented May 3, 2017 via email

@DavidMertz
Copy link
Author
DavidMertz commented May 4, 2017

I've pushed the small change in the review of #16 that is relevant to this PR. If you are fine with this approach rather than the opt-in approach, just merge this (but close without merging #16 and #17)

< 8000 /task-lists>

@mrocklin
Copy link
Contributor

OK to close this?

@DavidMertz
Copy link
Author

I still think it would be good to coerce lists/tuples as NumPy and scipy.sparse do. Closing the keyword opt-in in #17 seems reasonable since neither of you liked it. It's up to you, of course.

If you want this, let me rebase this against the accepted PR and remove the unnecessary change history. The diff should just be a few lines that way.

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.

4 participants
0