Skip to content

ARROW-6624: [C++][Python] Add SparseTensor.ToTensor() method #5539

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

ARROW-6624: [C++][Python] Add SparseTensor.ToTensor() method #5539

rok wants to merge 3 commits into from

Conversation

@rok
Copy link
Member

@rok rok commented Sep 29, 2019

This is to solve ARROW-6624.

@rok rok force-pushed the ARROW-6624 branch 2 times, most recently from 8f8594d to a7a9248 Compare October 7, 2019 02:13
@rok rok changed the title [WIP] ARROW-6624: [C++] Add SparseTensor.ToTensor() method [ARROW-6624: [C++] Add SparseTensor.ToTensor() method Oct 7, 2019
@rok rok force-pushed the ARROW-6624 branch 14 times, most recently from 7c9a5d5 to 899ac02 Compare October 10, 2019 20:53
@rok rok changed the title [ARROW-6624: [C++] Add SparseTensor.ToTensor() method ARROW-6624: [C++] Add SparseTensor.ToTensor() method Oct 10, 2019
@rok rok marked this pull request as ready for review October 10, 2019 23:42
@github-actions
Copy link

github-actions bot commented Oct 10, 2019

https://issues.apache.org/jira/browse/ARROW-6624

@rok
Copy link
Member Author

rok commented Oct 10, 2019

@mrkn we discussed this earlier (#4446 (comment)). Can you please review if this proposal makes sense?

@rok rok force-pushed the ARROW-6624 branch 3 times, most recently from e66814d to 8c01cac Compare October 12, 2019 14:43
@codecov-io
Copy link

codecov-io commented Oct 12, 2019 •
edited
Loading

Codecov Report

Merging #5539 into master will increase coverage by 19.19%.
The diff coverage is 97.95%.

Impacted file tree graph

@@             Coverage Diff             @@
##           master    #5539       +/-   ##
===========================================
+ Coverage    70.3%    89.5%   +19.19%     
===========================================
  Files         785      797       +12     
  Lines       95472   119114    +23642     
  Branches     1501        0     -1501     
===========================================
+ Hits        67123   106614    +39491     
+ Misses      27984    12500    -15484     
+ Partials      365        0      -365
Impacted Files Coverage Δ
cpp/src/arrow/python/numpy_convert.cc 95.06% <ø> (ø) ⬆️
python/pyarrow/tensor.pxi 79.25% <100%> (+0.63%) ⬆️
python/pyarrow/tests/test_sparse_tensor.py 100% <100%> (ø) ⬆️
cpp/src/arrow/sparse_tensor.h 100% <100%> (+2.94%) ⬆️
cpp/src/arrow/sparse_tensor_test.cc 100% <100%> (ø)
cpp/src/arrow/sparse_tensor.cc 98.57% <96.66%> (+9.17%) ⬆️
cpp/src/arrow/testing/gtest_util.h 97.36% <0%> (-2.64%) ⬇️
cpp/src/arrow/compute/kernel.h 60.57% <0%> (-2.11%) ⬇️
python/pyarrow/plasma.py 58.9% <0%> (-1.37%) ⬇️
cpp/src/arrow/result.h 90.38% <0%> (-1.29%) ⬇️
... and 664 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d923462...0eb11c6. Read the comment docs.

@rok rok changed the title ARROW-6624: [C++] Add SparseTensor.ToTensor() method ARROW-6624: [C++][Python] Add SparseTensor.ToTensor() method Oct 12, 2019
@pitrou
Copy link
Member

pitrou commented Oct 16, 2019

@mrkn Would you have time to review this? You are more acquainted with the sparse formats than I am.

@mrkn
Copy link
Member

mrkn commented Oct 16, 2019

@rok @pitrou I’ll review this today.

mrkn
mrkn requested changes Oct 17, 2019
View reviewed changes
Copy link
Member

@mrkn mrkn left a comment

Choose a reason for hiding this comment

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

Thank you for doing this. I left some comments. Please check them.

@rok rok force-pushed the ARROW-6624 branch 5 times, most recently from bad2aeb to f35b7af Compare October 17, 2019 20:03
@rok
Copy link
Member Author

rok commented Oct 17, 2019

Thanks for the review @mrkn :).
I've implemented your suggestions and also rebased on master to get the new tensor names (#5605).

mrkn
mrkn requested changes Oct 18, 2019
View reviewed changes
Copy link
Member

@mrkn mrkn left a comment

Choose a reason for hiding this comment

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

I added some comments. Please check them.

@rok rok force-pushed the ARROW-6624 branch 2 times, most recently from bac80d5 to 9b02d5a Compare October 18, 2019 10:48
@rok
Copy link
Member Author

rok commented Oct 18, 2019

@mrkn thanks for the feedback :). I've made changes, please review.

@rok rok force-pushed the ARROW-6624 branch 5 times, most recently from 1556be6 to 984e769 Compare October 19, 2019 10:41
@mrkn
Copy link
Member

mrkn commented Oct 20, 2019

+1 approved.
I'll merge this.

@mrkn mrkn closed this in 16e2667 Oct 20, 2019
@mrkn
Copy link
Member

mrkn commented Oct 20, 2019

@rok Thank you very much!

@rok
Copy link
Member Author

rok commented Oct 20, 2019

Thanks for your help @mrkn! :)

@asfimport asfimport mentioned this pull request Oct 20, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

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

4 participants

@rok @codecov-io @pitrou @mrkn