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

Expand expect_vector_equivalent to handle std::vector of vec_2d<T> and move traits out of detail #669

Merged

Conversation

harrism
Copy link
Member

@harrism harrism commented Sep 13, 2022

Description

  • Expands the test utility expect_vector_equivalent to support comparing vectors of vec_2d<T> with approximate comparison of floating point equality (using GTEST functionality). This function now also explicitly copies all vectors to the host for comparison.
  • Updates various tests to use the utility instead of directly calling EXPECT_EQ.
  • Moves traits.hpp to top-level include directory and moves all traits helpers out of detail namespace so they are more easily used in tests and user code.

TODO:

- [ ] coordinate_transform_test fails with this change, so I have not committed it. Something is different with EXPECT_EQ and this function for that test. Investigating. This will be fixed in a followup PR -- we discovered the test is flawed.

Checklist

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

@harrism harrism added tests Relating to tests and test automation improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels Sep 13, 2022
@harrism harrism self-assigned this Sep 13, 2022
@harrism harrism requested a review from a team as a code owner September 13, 2022 08:50
@github-actions github-actions bot added the libcuspatial Relates to the cuSpatial C++ library label Sep 13, 2022
@harrism harrism requested review from isVoid and removed request for zhangjianting September 13, 2022 08:51
@harrism harrism added the 3 - Ready for Review Ready for review by team label Sep 13, 2022
cpp/tests/utility/vector_equality.hpp Outdated Show resolved Hide resolved
cpp/tests/utility/vector_equality.hpp Outdated Show resolved Hide resolved
@harrism harrism changed the title Expand expect_vector_equivalent to handle std::vector of vec_2d<T> Expand expect_vector_equivalent to handle std::vector of vec_2d<T> and move traits out of detail Sep 14, 2022
@isVoid
Copy link
Contributor

isVoid commented Sep 14, 2022

Lots of nice refactoring of old test code too... Nice improvement.

@harrism
Copy link
Member Author

harrism commented Sep 14, 2022

@gpucibot merge

@rapids-bot rapids-bot bot merged commit 29307b0 into rapidsai:branch-22.10 Sep 14, 2022
rapids-bot bot pushed a commit that referenced this pull request Sep 21, 2022
Converting from `EXPECT_EQ` to `expect_vector_equivalent` (#669) caused `coordinate_transform_test` to fail due to floating point differences. It's strange that it passed with exact equality testing (perhaps a problem in Thrust?). In any case, this fixes the test data generation and DRYs up the tests a bit.

Authors:
  - Mark Harris (https://github.com/harrism)

Approvers:
  - H. Thomson Comer (https://github.com/thomcom)
  - Michael Wang (https://github.com/isVoid)

URL: #671
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3 - Ready for Review Ready for review by team improvement Improvement / enhancement to an existing function libcuspatial Relates to the cuSpatial C++ library non-breaking Non-breaking change tests Relating to tests and test automation
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants