-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-4335: [C++] Better document sparse tensor support #3810
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
Conversation
cpp/src/arrow/sparse_tensor.h
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I put this class in internal namespace to remove it from documentation targets.
cpp/src/arrow/sparse_tensor.h
Outdated
| /// values in sparse tensor | ||
| /// \brief EXPERIMENTAL: The base class for the index of a sparse tensor | ||
| /// | ||
| /// SparseIndex are used to represent where is non-zero values in a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is less clear than it could be, maybe rewrite as "SparseIndex describes where the non-zero elements are within a SparseTensor."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rewrote the sentence.
cpp/src/arrow/sparse_tensor.h
Outdated
| /// SparseIndex are used to represent where is non-zero values in a | ||
| /// SparseTensor. | ||
| /// | ||
| /// There are several ways of the index representation. The format_id is used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it'd be useful to note specifically that there is a single derived class for each possible value of format_id (SparseTensorFormat::COO -> SparseCOOIndex and SparseTensorFormat::CSR -> SparseCSRIndex)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried to describe that you mentioned.
Could you please review it again?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Also this sentence isn't quite grammatically correct. Could you use -ways of the index representation+ways to represent this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for your feedback. I fixed the incorrect representation.
|
The Travis failure looks spurious to me |
|
@kou Could you understand what is happened on CI here? |
|
Umm. I didn't see this SEGV. |
Codecov Report
@@ Coverage Diff @@
## master #3810 +/- ##
===========================================
+ Coverage 67.76% 87.79% +20.02%
===========================================
Files 468 710 +242
Lines 58721 86896 +28175
Branches 1252 1252
===========================================
+ Hits 39793 76287 +36494
+ Misses 18811 10496 -8315
+ Partials 117 113 -4
Continue to review full report at Codecov.
|
|
I've fixed the SEGV. |
wesm
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1. Thanks all!
I wrote descriptions for sparse tensor classes. Author: Kenta Murata <mrkn@mrkn.jp> Closes apache#3810 from mrkn/sparse_tensor_doc and squashes the following commits: 9c4fe14 <Kenta Murata> Fix incorrect English 522a982 <Kenta Murata> Modify comments following review comments 83cab03 <Kenta Murata> Put SparseIndexBase<> class in internal namespace 6cd13e2 <Kenta Murata> Write descriptions of sparse tensor
I wrote descriptions for sparse tensor classes.