Skip to content
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

ARROW-5813: [C++] Fix TensorEquals for different contiguous tensors #4774

Closed

Conversation

mrkn
Copy link
Member

@mrkn mrkn commented Jul 2, 2019

This change makes TensorEquals correctly calculate the equality of a row-major tensor and a column-major tensor.

@mrkn mrkn force-pushed the tensor_equals_for_different_contiguous branch from e1b9b9e to e7ef17d Compare July 2, 2019 00:40
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.

Looks good on the principle, but I think testing should be a bit more comprehensive perhaps?

@@ -155,6 +155,49 @@ TEST(TestTensor, CountNonZeroForNonContiguousTensor) {
AssertCountNonZero(t, 8);
}

TEST(TestTensor, Equals) {
Copy link
Member

Choose a reason for hiding this comment

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

Is it possible to also test unequal tensors? Contiguous, non-contiguous...

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, I forgot to write unequal cases.
I added them at d40c3c7

@pitrou
Copy link
Member

pitrou commented Jul 2, 2019

By the way, memcmp is problematic for floating-point data (because of NaNs at least, and possibly denormals - but not sure we care about the latter). That can be tackled in a separate JIRA though.

@mrkn
Copy link
Member Author

mrkn commented Jul 3, 2019

I filed ARROW-5830.

@codecov-io
Copy link

Codecov Report

Merging #4774 into master will increase coverage by 2.64%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #4774      +/-   ##
==========================================
+ Coverage   86.42%   89.06%   +2.64%     
==========================================
  Files         994      719     -275     
  Lines      138595    99883   -38712     
  Branches     1418        0    -1418     
==========================================
- Hits       119776    88960   -30816     
+ Misses      18457    10923    -7534     
+ Partials      362        0     -362
Impacted Files Coverage Δ
cpp/src/arrow/tensor-test.cc 100% <100%> (ø) ⬆️
cpp/src/arrow/compare.cc 86.12% <100%> (+6.12%) ⬆️
cpp/src/arrow/python/pyarrow.cc 23.91% <0%> (-5.82%) ⬇️
python/pyarrow/public-api.pxi 57.43% <0%> (-4.11%) ⬇️
python/pyarrow/array.pxi 80.61% <0%> (-0.25%) ⬇️
python/pyarrow/__init__.py 40.54% <0%> (ø) ⬆️
cpp/src/plasma/common.cc 88.46% <0%> (ø) ⬆️
cpp/src/parquet/column_reader.cc 91.5% <0%> (ø) ⬆️
cpp/src/parquet/encoding.cc 93.99% <0%> (ø) ⬆️
cpp/src/parquet/arrow/reader.cc 85.71% <0%> (ø) ⬆️
... and 291 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 3cdd0cf...d40c3c7. Read the comment docs.

@pitrou
Copy link
Member

pitrou commented Jul 3, 2019

Thanks @mrkn ! Will merge.

@pitrou pitrou closed this in 86f0a3a Jul 3, 2019
@mrkn mrkn deleted the tensor_equals_for_different_contiguous branch July 4, 2019 01:04
kou pushed a commit that referenced this pull request Jul 4, 2019
This change makes TensorEquals correctly calculate the equality of a row-major tensor and a column-major tensor.

Author: Kenta Murata <[email protected]>

Closes #4774 from mrkn/tensor_equals_for_different_contiguous and squashes the following commits:

d40c3c7 <Kenta Murata> Add unequal expectations
e7ef17d <Kenta Murata> Fix TensorEquals for different contiguous tensors
kszucs pushed a commit that referenced this pull request Jul 22, 2019
This change makes TensorEquals correctly calculate the equality of a row-major tensor and a column-major tensor.

Author: Kenta Murata <[email protected]>

Closes #4774 from mrkn/tensor_equals_for_different_contiguous and squashes the following commits:

d40c3c7 <Kenta Murata> Add unequal expectations
e7ef17d <Kenta Murata> Fix TensorEquals for different contiguous tensors
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.

3 participants