Bugfix/cosinus-overflow

[Klummel69]

Hello Mike

Currently I am working on the trigonometric functions (sin,cos,tan) of fpm. I am creating an optimized version that has (for FractionBits ≥ 16) smaller deviation , with presumably the same performance.

I noticed that the cosine function uses an addition:

return sin(fixed<B, I, F, R>::half_pi() + x);

An overflow is not detected. Due to the overflow, wrong values are calculated in some cases. This could happen especially for data types with few digits before the decimal point (fixed_8_24...). Therefore a suggestion for a bugfix:

if (x > Fixed(0)) {  // Prevent an overflow due to the addition of π/2
        return sin(x - (Fixed::two_pi() - Fixed::half_pi()));
    } else {
        return sin(Fixed::half_pi() + x);
    }

I have adapted the test in trigonometry.cpp accordingly. It now also tests the boundary raw values (INT32_MIN, INT32_MAX) and a value that has a deviation of 200% due to the error (for fixed_16_16). See code part "// Boundary-value analysis"

Note: in this code part I have set an error limit of ``MAX_ERROR_PERC = 0.034'', because the cosine values have a larger error over the whole range of values than in the interval [-π ... +π]. The same applies to tan(x).

[Klummel69]

Addendum: for the trigonometric functions specifying/checking a relative error makes not much sense. I would only consider the absolute error for these functions. Since the value for sin/cos is in the range [-1...+1] anyway, you already have a "normalized" error value.

Example: If you set "27.001" as angle in the TEST(trigonometry, cos), the following values result:

angle = 27.001° 
flt_angle = 4.7125635133
cos_fixed = 0.0001831
cos_real  = 0.0001745
diff_absolute = 8.57e-06
diff_relative = 0.049 = 4.9%

Here the relative error makes no sense.

For example, if you want to convert Polar Coordinates to a Cartesian coordinate system, I would use the max. diff_absolute and not diff_relative for error estimation:

x = r * sin(φ)
y = r * cos(φ)

Δx = r * max_diff_absolute
Δy = r * max_diff_absolute

Greeting Klaus

P.S. The more I look into fpm, the more excited I get about it. Great work!

[MikeLankamp]

Addendum: for the trigonometric functions specifying/checking a relative error makes not much sense.

I'm not sure about this. A cosine result of 0.0002 instead of 0.0001 has a very small absolute error. But if you rely on that value being 0.0001, that's a very large relative error, so I think being able to bound the relative error as well says something about the accuracy of the cosine for small resulting values.

P.S. The more I look into fpm, the more excited I get about it. Great work!

Glad you like it! I appreciate you and others contributing to improve it.

[Klummel69]

Also, could you please update the commit message to use conventional commits, i.e. a title of "fix: cosine could cause overflow" and a short description of the problem and solution in the commit body?

Sorry, did not think about it.

Question: So far I have only changed commit messages before starting a pull-request. Does Github adjust the pull-request correctly?

[Klummel69]

I'm not sure about this. A cosine result of 0.0002 instead of 0.0001 has a very small absolute error. But if you rely on that value being 0.0001, that's a very large relative error, so I think being able to bound the relative error as well says something about the accuracy of the cosine for small resulting values.

But then you should also test for both error types (relative and absolute). The limit is currently set constexpr auto MAX_ERROR_PERC = 0.002; This corresponds to a max. relative error of 0.2%

In reality, the relative error (except at the 1/2π and 3/2π points) is less than 0.05%. See diagram. The peaks become larger the finer the steps are chosen. The limit value of 0.002 is only reached because the test resolution is roughly selected.

[MikeLankamp]

Does Github adjust the pull-request correctly?

Yes, it will. Just amend the commits (with e.g. git commit amend or git rebase) and force-push the updated commit(s) in the branch.

The limit value of 0.002 is only reached because the test resolution is roughly selected.

That is a fair point. Relative errors for those points near 0 are a pain anyway. Allright, I suppose given that this test is meant to catch regressions and that the absolute error is pretty small, I'm fine with changing cos/sin tests to absolute errors instead. I'd really appreciate if you'd be willing to do this (in another PR).

[Klummel69]

I'd really appreciate if you'd be willing to do this (in another PR).

Sure, I'm doing it in the course of improving sin/cos. But it will take a few more days. I create a new branch "improve-sin-cos-accuracy" Additionally, I'm creating a new 'accuracy' document (as a basis for discussion).

Klaus

[Klummel69]

Does Github adjust the pull-request correctly?
Yes, it will. Just amend the commits (with e.g. git commit amend or git rebase) and force-push the updated commit(s) in the branch.

After the 4th attempt it worked, the terminal used discarded the edited commit message.