tresinformal / drakkar

The tresinformal video game called 'Drakkar'
GNU General Public License v3.0
11 stars 4 forks source link

Equality between `double`s can be checked, with some tolerance #594

Closed TheoPannetier closed 1 year ago

TheoPannetier commented 1 year ago

doubles (i.e., numbers with decimal values) can be checked for equality with x == y, but this will only return TRUE if both numbers are EXACTLY equal. Because of rounding error or other forms of imprecision, often in tests we are forced to use something like abs(x - y) < 0.0001, that is the difference is smaller than some level of tolerance, which we tend to define arbitrarily and for every test.

Instead, it would be convenient for tests to be able to call a function that checks for this, with a preset level of tolerance.

  1. Create a test for such a function are_equal() that would return TRUE if the difference between two doubles is smaller than some tolerance
  2. Create this function :)
  3. Replace all occurrences of tests of equality between doubles with this function.

You can make the level of tolerance hard-coded in the function definition, an argument of the function, or both, whatever you think is more advisable. You're boss!

EvoLandEco commented 1 year ago

Ah, seems like we posted similar issues at the same time, I'll close mine and make it a comment to your issue

It was an idea raised when we were reviewing @TheoPannetier 's code yesterday. Throughout the code base there exist various method in the tests to test whether the error produced (due to computer arithmetic errors) is within the tolerance.

Example 1

assert(predicted_player_position.get_y() - get_y(p) < 0.001
   &&predicted_player_position.get_y() - get_y(p) > -0.001);

Example 2

double error_x = abs(get_x(c) - default_speed);
double error_y = abs(get_y(c) - 0);
assert(error_x < 0.00001);
assert(error_y < 0.00001);

Our opinion is that we should standardize the method, as well as the tolerance value. To make it easier, we might need to implement a utility function to do the job, to save a lot of time of people writing redundant code.

TheoPannetier commented 1 year ago

Ah, that's unfortunate! What were the chances that we were both writing the same issue at the same time 😂

EvoLandEco commented 1 year ago

I plan to implement are_equal() as following:

bool are_equal(const double a, const double b, double tolerance)
{
    return fabs(a - b) < tolerance
}

But where should I place it? I can't find a proper source file. Do we need to create a new file like test_functions.h/cpp to store those functions that might be reused for testing purposes?

Another thing is the choice between std::abs()s and fabs() when we are trying to return the absolute value of a float/double, I assume fabs() is safer here?

TheoPannetier commented 1 year ago

Good question! We have put the equally generic calc_var in game.cpp, so you could place it next to it. But I definitely see the logic for creating a separate file for such non-class related functions. You're boss here! I would not call a new file test_functions however, may be confusing with respect to actual tests. Maybe math / utils / calculus / something else?

std::abs and fabs() should both do the job. I would prefer std::abs because it is immediately obvious what it does, so better readability. fabs is more restrictive, strictly expecting a floating point number as input, which would be a good thing outside the body of a function. But here, the declared arguments already enforce that a and b are double anyways :)

EvoLandEco commented 1 year ago

Good question! We have put the equally generic calc_var in game.cpp, so you could place it next to it. But I definitely see the logic for creating a separate file for such non-class related functions. You're boss here! I would not call a new file test_functions however, may be confusing with respect to actual tests. Maybe math / utils / calculus / something else?

std::abs and fabs() should both do the job. I would prefer std::abs because it is immediately obvious what it does, so better readability. fabs is more restrictive, strictly expecting a floating point number as input, which would be a good thing outside the body of a function. But here, the declared arguments already enforce that a and b are double anyways :)

It happened several times that we accidentally used abs() instead of std::abs(), I still remember that the tests were passing on Windows but failing on Linux. My intention here is to avoid such kind of mistake, but of course, std::abs looks more self-explanatory.

EvoLandEco commented 1 year ago

The following tests shall pass: void test_utils()

{
    assert(are_equal(0.0, 0.0));
    assert(!are_equal(0.0, 0.1));
    assert(!are_equal(0.00001, 0.00002, 0.000001));
    assert(are_equal(0.0001, 0.0002, 0.001));
}
EvoLandEco commented 1 year ago

closed by #647