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

Augment Cuspatial Test Utility to Allow User Specified Abs Error #752

Merged
merged 3 commits into from
Nov 1, 2022

Conversation

isVoid
Copy link
Contributor

@isVoid isVoid commented Oct 26, 2022

Description

This PR augments Cuspatial Vector Equality Test Utility with an extra parameter to allow user pass in pre-defined absolute error.
In many cases, cuspatial has to compare results with external libraries such as shapely or gdal. Since cuspatial allows user to pick computation precision based on input data (not a bug!), requiring the result to match external libraries within 4ULP is often too stringent, as external libraries often perform computation in different precisions. Adding the flexibility to specifiy abs error in test utilitiy makes it easy for developer to pick desired error range based on specific use cases.

This PR also introduces a test macro that shows lineno of failed tests.

Checklist

  • I am familiar with the Contributing Guidelines.
  • New or existing tests cover these changes.
  • The documentation is up to date with these changes.

@isVoid isVoid requested a review from a team as a code owner October 26, 2022 00:28
@isVoid isVoid requested review from trxcllnt, zhangjianting and harrism and removed request for trxcllnt and zhangjianting October 26, 2022 00:28
@github-actions github-actions bot added the libcuspatial Relates to the cuSpatial C++ library label Oct 26, 2022
@isVoid isVoid added feature request New feature or request 3 - Ready for Review Ready for review by team non-breaking Non-breaking change labels Oct 26, 2022
@@ -69,6 +95,20 @@ MATCHER(float_matcher, std::string(negation ? "are not" : "are") + " approximate
return false;
}

MATCHER_P(float_near_matcher,
Copy link
Member

Choose a reason for hiding this comment

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

The float_matcher is also a "near" matcher, so perhaps float_near_matcher is not the right name. The only different is that we pass abs_error. GTEST ignores the abs error if it is zero. So why not just augment the existing matchers with default abs_error of zero to avoid all the additional code?

Copy link
Contributor Author

@isVoid isVoid Oct 28, 2022

Choose a reason for hiding this comment

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

GTEST ignores the abs error if it is zero.

Hmm, it's not that simple.. Your assumption is that using FloatNear(abs_error==0) is the same as using FloatEq.
Technically they go through different code paths.

FLOAT_NEAR(abs_error==0) will go to this:
https://github.com/google/googletest/blob/3026483ae575e2de942db5e760cf95e973308dd5/googlemock/include/gmock/gmock-matchers.h#L1687

which will not use the ULP near equal check but completely resort to abs_err check.

FLOAT_EQ matcher will use this:
https://github.com/google/googletest/blob/3026483ae575e2de942db5e760cf95e973308dd5/googlemock/include/gmock/gmock-matchers.h#L1706

which will use the ULP near equal check.

While we can default abs_error to -1, the default value where gtest use internally to differentiate, I believe that's an implementation detail and it's in nature an invalid input to the matcher.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If just for naming, we can call it float_eq_by_ulp / float_eq_by_abs_error. As for the user facing API expect_vector_equivalent, I think we should keep it as is and use overloads for user to differentiate.

@isVoid isVoid added 5 - Ready to Merge Testing and reviews complete, ready to merge and removed 3 - Ready for Review Ready for review by team labels Nov 1, 2022
@isVoid
Copy link
Contributor Author

isVoid commented Nov 1, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 92c3a08 into rapidsai:branch-22.12 Nov 1, 2022
rapids-bot bot pushed a commit that referenced this pull request Nov 7, 2022
…nge`, adds support to multilinestring distance (#755)

Note, this is the first part of `pairwise_linestring_distance` refactoring, part 1 of PR: #753

Depends on #752 

Contributes to #706, #703

Closes #745

Authors:
  - Michael Wang (https://github.com/isVoid)

Approvers:
  - H. Thomson Comer (https://github.com/thomcom)
  - Mark Harris (https://github.com/harrism)

URL: #755
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to Merge Testing and reviews complete, ready to merge feature request New feature or request libcuspatial Relates to the cuSpatial C++ library non-breaking Non-breaking change
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants