Skip to content

ARROW-4223: [Python] Support scipy.sparse integration #4779

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
Closed
wants to merge 16 commits into from
Closed

ARROW-4223: [Python] Support scipy.sparse integration #4779

wants to merge 16 commits into from

Conversation

@rok
Copy link
Member

@rok rok commented Jul 2, 2019

This is to resolve ARROW-4223.

@kszucs kszucs changed the title ARROW-4223 [Python] Support scipy.sparse integration ARROW-4223: [Python] Support scipy.sparse integration Jul 4, 2019
jorisvandenbossche
jorisvandenbossche reviewed Jul 4, 2019
View reviewed changes
Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

@rok I know the PR is still indicated as draft, so all comments I made you probably were still going to do, but while looking at the diff, thought to at once comment

@rok rok force-pushed the ARROW-4223 branch 2 times, most recently from 5b7472b to eca7a70 Compare July 8, 2019 21:51
@rok rok force-pushed the ARROW-4223 branch 10 times, most recently from ff60f58 to 75097f7 Compare July 20, 2019 16:50
@rok rok marked this pull request as ready for review July 20, 2019 17:02
@rok
Copy link
Member Author

rok commented Jul 20, 2019 •
edited
Loading

I think this is ready for review.
Appveyor seems to be failing on unrelated issue.

@rok rok force-pushed the ARROW-4223 branch 2 times, most recently from 081032b to 2ca9ed7 Compare July 22, 2019 18:12
@rok rok force-pushed the ARROW-4223 branch 5 times, most recently from 15b973e to ebbbdf2 Compare July 28, 2019 23:44
@rok rok force-pushed the ARROW-4223 branch 2 times, most recently from d812904 to 54ad5ad Compare November 11, 2019 19:40
@rok
Copy link
Member Author

rok commented Nov 11, 2019

@ursabot build

@rok
Copy link
Member Author

rok commented Nov 11, 2019

@pitrou I think this is now ready for another review round. I've updated #4990 too.

pitrou
pitrou approved these changes Nov 12, 2019
View reviewed changes
Copy link
Member

@pitrou pitrou left a comment

Choose a reason for hiding this comment

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

LGTM, just one question.

python/pyarrow/tests/test_sparse_tensor.py Outdated
Copy link
Member

@pitrou pitrou Nov 12, 2019

Choose a reason for hiding this comment

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

What is the purpose of the astype() calls here?

Copy link
Member Author

@rok rok Nov 12, 2019

Choose a reason for hiding this comment

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

This is a workaround for scipy bug (or lack of feature) see this issue.

Copy link
Member

@pitrou pitrou Nov 12, 2019

Choose a reason for hiding this comment

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

But you have if dtype_str != 'f2' above.

Copy link
Member Author

@rok rok Nov 12, 2019

Choose a reason for hiding this comment

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

Oh, yeah.. this probably should have failed. That's odd. I'll look into it tonight.

Copy link
Member Author

@rok rok Nov 12, 2019

Choose a reason for hiding this comment

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

Fixed.

jorisvandenbossche
jorisvandenbossche reviewed Nov 12, 2019
View reviewed changes
Copy link
Member

@jorisvandenbossche jorisvandenbossche left a comment

Choose a reason for hiding this comment

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

minor nit

python/pyarrow/tensor.pxi Outdated
Copy link
Member

@jorisvandenbossche jorisvandenbossche Nov 12, 2019

Choose a reason for hiding this comment

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

Could check here that the passed object is actually a scipy.sparse.coo_matrix ?
(right now the user will probably get an attribute error if you pass a different scipy matrix type)

(and the same for the other from_scipy below)

Copy link
Member Author

@rok rok Nov 12, 2019

Choose a reason for hiding this comment

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

Done.

Copy link
Member

@jorisvandenbossche jorisvandenbossche Nov 12, 2019

Choose a reason for hiding this comment

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

Thanks!

Copy link
Member Author

@rok rok Nov 12, 2019

Choose a reason for hiding this comment

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

Thank you :)

rok and others added 16 commits November 13, 2019 09:45
@rok
Adding scipy.sparse integration.
247cdbd
@rok
Changes to GetSparseTensorMessage. Enabling comparison for SparseTens…
c54f005 
…or roundtrip test.
@rok
Changes to GetSparseTensorMessage to enable SparseTensor to component…
958b354 
…s serialization.
@mrkn @rok
Prevent copying buffers on component serialization of a SparseTensor
56df620
@rok
Temporarily disabling test_sparse_tensor_csr_components_serialization…
b13604d 
… test.
@rok
Adding from_scipy and to scipy methods to SparseTensorCOO and SparseT…
11068eb 
…ensorCSR.
@rok
Re-enabling test_sparse_tensor_coo_components_serialization.
2bc2534
@rok
Applying review feedback for python tests.
db1fb5a
@rok
Enabling serialization with pydata/sparse.
f4d2e1e
@mrkn @rok
Extract a common part from ReadSparseTensorPayload and ReadSparseTensor
90cbadf
@mrkn @rok
Add SparseCOOIndex::Make
b06429d
@mrkn @rok
Add SparseCSRIndex::Make
927bf5d
@rok
Fixing deserialization issue. Rebasing for new tensor type names.
22e864d
@rok
Implementing review feedback.
4d7d2b0
@rok
Fixing scipy->sparse_tensor tests for dtype=f2.
d5484bf
@rok
Adding type check to from_scipy.
eca3885
@pitrou
Copy link
Member

pitrou commented Nov 13, 2019

The R download failure on AppVeyor is unrelated, merging.

@pitrou pitrou closed this in cdaf988 Nov 13, 2019
@rok
Copy link
Member Author

rok commented Nov 13, 2019

Thanks @pitrou @mrkn and @jorisvandenbossche! :)

@rok rok deleted the ARROW-4223 branch November 13, 2019 10:39
pitrou pushed a commit that referenced this pull request Nov 13, 2019
@rok @pitrou
ARROW-4224: [Python] Support integration with pydata/sparse library
d286edb 
This is to resolve [ARROW-4224](https://issues.apache.org/jira/browse/ARROW-4224).

Please note this requires #4779 to be reviewed and merged first.

Closes #4990 from rok/ARROW-4224 and squashes the following commits:

d99d52d <Rok> Implementing review feedback.
4efdd35 <Rok> Adding type check to from_pydata_sparse.
759e0b3 <Rok> Adding from_pydata_sparse and to_pydata_sparse.

Authored-by: Rok <rok@mihevc.org>
Signed-off-by: Antoine Pitrou <antoine@python.org>
@evansd evansd mentioned this pull request Feb 10, 2020
Closed
@asfimport asfimport mentioned this pull request Nov 13, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mrkn mrkn mrkn left review comments

@jorisvandenbossche jorisvandenbossche jorisvandenbossche left review comments

@pitrou pitrou pitrou 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.

5 participants

@rok @codecov-io @mrkn @pitrou @jorisvandenbossche