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

Use std::atan (::atan is double-only) #1553

Closed
wants to merge 1 commit into from

Conversation

WOnder93
Copy link

::atan is the C version of the atan function and is not overloaded (it only works with doubles, ::atanf is for floats). Therefore the current code first casts the score to float, divides it by 128.0F, then converts it to double and the rest of the computation is done with double precision. The cast to float indicates that this was not the intended behavior (and it is also a waste).

This patch switches tostd::atan, which is the C++ version of this function and has overloads for both float and double and thus behaves as expected.

This seems to be a non-functional change (bench stays the same), although the resulting values might be slightly different in some corner case.

Compiler Explorer link (for assembly output difference)

@Stefano80
Copy link
Contributor

What problems are solved by this patch exactly?

@WOnder93
Copy link
Author

Current code is equivalent to:

float step1 = float(previousScore) / 128;
double step2 = double(step1);
double step3 = std::atan(step2);
double step4 = 48 * step3;
double step5 = std::round(step4);
ct += int(step5);

while the new code is simply:

float step1 = float(previousScore) / 128;
float step2 = std::atan(step1);
float step3 = 48 * step2;
float step4 = std::round(step3);
ct += int(step4);

@ceebo
Copy link

ceebo commented Apr 18, 2018

We could avoid conversions to float altogether using the expression:

ct += 88 * previousScore / (std::abs(previousScore) + 200);

It approximates the expression in master to within +/-5 over the range [-1000,1000] but it would be a functional change and require proper testing.

@gonlem
Copy link

gonlem commented Apr 18, 2018

Ceebo's formula don't use trigonometric function and is (subjectively) easier to understand. His formula is probably computationally faster than the atan version. Could it be considered as a simplification ?

image

https://docs.google.com/spreadsheets/d/1jPfg0sZeX-zgjxn9J9B9M4bNgsXh0740wDjPGO1oOIM/edit?usp=sharing

@vondele
Copy link
Member

vondele commented Apr 18, 2018

@ceebo I think your expression should be tested with [-3,1] bounds, getting rid of the floats and conversions altogether is a simplification in my opinion.

@snicolet
Copy link
Member

The discussion has shifted to an alternative formula for dynamic contempt (again ?!?), but the use of atan is fine in current master, in my opinion. So I am closing this pull request about the use of "std::atan" against "atan".

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.

6 participants