Skip to content

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

Closed
rok wants to merge 3 commits into
apache:masterfrom
rok:ARROW-6624
Closed

ARROW-6624: [C++][Python] Add SparseTensor.ToTensor() method#5539
rok wants to merge 3 commits into
apache:masterfrom
rok:ARROW-6624

Conversation

@rok

@rok rok commented Sep 29, 2019

Copy link
Copy Markdown
Member

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
Copy Markdown

@rok

rok commented Oct 10, 2019

Copy link
Copy Markdown
Member Author

@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

codecov-io commented Oct 12, 2019

Copy link
Copy Markdown

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

pitrou commented Oct 16, 2019

Copy link
Copy Markdown
Member

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

@mrkn

mrkn commented Oct 16, 2019

Copy link
Copy Markdown
Member

@rok @pitrou I’ll review this today.

@mrkn mrkn left a comment

Copy link
Copy Markdown
Member

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.

Comment thread cpp/src/arrow/sparse_tensor.cc Outdated
Comment thread cpp/src/arrow/sparse_tensor.h Outdated
Comment thread cpp/src/arrow/sparse_tensor.cc Outdated
@rok rok force-pushed the ARROW-6624 branch 5 times, most recently from bad2aeb to f35b7af Compare October 17, 2019 20:03
@rok

rok commented Oct 17, 2019

Copy link
Copy Markdown
Member Author

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

@mrkn mrkn left a comment

Copy link
Copy Markdown
Member

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.

Comment thread cpp/src/arrow/sparse_tensor_test.cc Outdated
Comment thread cpp/src/arrow/sparse_tensor_test.cc Outdated
Comment thread cpp/src/arrow/sparse_tensor.h Outdated
Comment thread cpp/src/arrow/sparse_tensor.cc Outdated
Comment thread cpp/src/arrow/sparse_tensor.cc Outdated
Comment thread cpp/src/arrow/sparse_tensor.cc Outdated
Comment thread cpp/src/arrow/sparse_tensor.cc Outdated
Comment thread cpp/src/arrow/sparse_tensor.cc Outdated
@rok rok force-pushed the ARROW-6624 branch 2 times, most recently from bac80d5 to 9b02d5a Compare October 18, 2019 10:48
@rok

rok commented Oct 18, 2019

Copy link
Copy Markdown
Member Author

@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

mrkn commented Oct 20, 2019

Copy link
Copy Markdown
Member

+1 approved.
I'll merge this.

@mrkn mrkn closed this in 16e2667 Oct 20, 2019
@mrkn

mrkn commented Oct 20, 2019

Copy link
Copy Markdown
Member

@rok Thank you very much!

@rok

rok commented Oct 20, 2019

Copy link
Copy Markdown
Member Author

Thanks for your help @mrkn! :)

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