Skip to content

Add equals function for comparing std::vector<double>#1069

Merged
BenjaminRodenberg merged 4 commits intoprecice:developfrom
BenjaminRodenberg:test-floating-point-vector
Aug 24, 2021
Merged

Add equals function for comparing std::vector<double>#1069
BenjaminRodenberg merged 4 commits intoprecice:developfrom
BenjaminRodenberg:test-floating-point-vector

Conversation

@BenjaminRodenberg
Copy link
Copy Markdown
Contributor

Main changes of this PR

  • Add boost::test_tools::predicate_result equals<DerivedA, DerivedB>(std::vector<DerivedA> &A, std::vector<DerivedB> &B, double tolerance = precice::math::NUMERICAL_ZERO_DIFFERENCE)
  • Don't use BOOST_TEST(vecA == vecB), but BOOST_TEST(testing::equals(vecA, vecB)) with some floating point tolerance.

Motivation and additional information

A BOOST_TEST(vecA == vecB) caused a lot of confusion in BenjaminRodenberg#1. We were lucky until now that floating point accuracy did not matter here, but it will as soon as interpolation (or something else) happens when data is exchanged.

Author's checklist

  • I added a changelog file with this PR number in docs/changelog/ if there are noteworthy changes.
  • I ran tools/formatting/check-format and everything is formatted correctly.
  • I sticked to C++14 features.
  • I sticked to CMake version 3.10.
  • I squashed / am about to squash all commits that should be seen as one.

Reviewers' checklist

  • Does the changelog entry make sense? Is it formatted correctly?
  • Do you understand the code changes?
  • (more questions/tasks)

@BenjaminRodenberg BenjaminRodenberg self-assigned this Aug 23, 2021
@BenjaminRodenberg BenjaminRodenberg marked this pull request as ready for review August 23, 2021 12:36
Copy link
Copy Markdown
Member

@davidscn davidscn left a comment

Choose a reason for hiding this comment

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

Only some general feedback comments. Feel free to merge.

@BenjaminRodenberg BenjaminRodenberg merged commit 63b22a2 into precice:develop Aug 24, 2021
@fsimonis
Copy link
Copy Markdown
Member

fsimonis commented Sep 6, 2021

@BenjaminRodenberg I am not very keen on this addition for some reasons:

  • Boost.Test already compares floating point numbers using a tolerance
  • The predicate doesn't show where the ranges differ
  • The predicate doesn't implement a "real" predicate
  • The predicate only needs one template parameter. Comparing std::vector<double> to std::vector<int> is a mistake in the test.
  • There is already a build-in tool for this:
     BOOST_TEST(a == b, boost::test_tools::per_element() );

@uekerman uekerman added this to the Version 2.3.0 milestone Sep 10, 2021
@BenjaminRodenberg
Copy link
Copy Markdown
Contributor Author

@BenjaminRodenberg I am not very keen on this addition for some reasons:

* Boost.Test already compares floating point numbers using [a tolerance](https://github.com/precice/precice/blob/cff1b7fff5b8c80e62a0e6c3404e4c045577cb4e/src/testing/main.cpp#L96)

* The predicate doesn't show where the ranges differ

* The predicate doesn't implement a ["real" predicate](https://www.boost.org/doc/libs/1_65_1/libs/test/doc/html/boost_test/testing_tools/custom_predicates.html)

* The predicate only needs one template parameter. Comparing `std::vector<double>` to `std::vector<int>` is a mistake in the test.

* There is already a [build-in tool](https://www.boost.org/doc/libs/1_65_1/libs/test/doc/html/boost_test/testing_tools/extended_comparison/collections.html) for this:
  ```c++
   BOOST_TEST(a == b, boost::test_tools::per_element() );
  ```

@fsimonis can you open an issue? I fear otherwise these comments might get lost.

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.

4 participants