ARROW-4320: [C++] Add tests for non-contiguous tensors #3453
Conversation
pitrou
left a comment
pitrou
left a comment
There was a problem hiding this comment.
Thanks for adding this! Here are some comments.
cpp/src/arrow/tensor-test.cc
Outdated
There was a problem hiding this comment.
Also ASSERT_FALSE(is_column_major()) and ASSERT_TRUE(is_contiguous())?
cpp/src/arrow/tensor-test.cc
Outdated
There was a problem hiding this comment.
Also ASSERT_FALSE on the row_major / column_major properties?
cpp/src/arrow/tensor-test.cc
Outdated
cpp/src/arrow/sparse_tensor-test.cc
Outdated
There was a problem hiding this comment.
Ideally there would be a test helper which would allow writing:
AssertCOOIndex(sidx, {0}, {0, 0, 0});
AssertCOOIndex(sidx, {1}, {0, 0, 2});
// etc.It would make the tests a bit clearer and shorter.
cpp/src/arrow/sparse_tensor-test.cc
Outdated
There was a problem hiding this comment.
Interesting. Out of curiosity, why not row-major?
There was a problem hiding this comment.
It is for the zero-copy compatibility with SciPy and pydata/sparse.
See this discussion.
cpp/src/arrow/sparse_tensor-test.cc
Outdated
There was a problem hiding this comment.
This kind of code occurs several time in the test file. Could you define a test helper for this (perhaps in test-util.h)? So this would become e.g.:
AssertNumericDataEqual(st.raw_data(), std::vector<int64_t>{1, 2, 3, 4, 5, 6, 11, 12, 13, 14, 15, 16});
cpp/src/arrow/sparse_tensor-test.cc
Outdated
There was a problem hiding this comment.
This could benefit from a AssertNumericDataEqual helper or similar.
|
Thanks for the updates @mrkn. There are some compilation and linting failures that need fixing (see the Travis-CI and AppVeyor builds). |
|
I will merge if CI succeeds. |
pitrou
force-pushed
the
add_strided_numeric_tensor_tests
branch
from
0ccadf4 to
5841794
Compare
|
I pushed a fix for what I hope is the last Windows compilation error :-) |
|
Successful AppVeyor build at: https://ci.appveyor.com/project/pitrou/arrow/builds/21945035 |
|
Thank you very much! |
2 participants
I would like to add some test cases for tensors with non-contiguous strides.