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 Mac for testing edge cases differing across processors #665

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

fulldecent
Copy link
Contributor

@fulldecent fulldecent commented Dec 23, 2024

Different processors/operating systems can produce different encoding results, due to different implementations of floating point operations.

This PR confirms adds a test of the C implementation using macos-latest, currently macos-14-arm64.

@drinckes
Copy link
Contributor

Can you change the PR title to describe what it is doing, and the comment to describe why please.

@fulldecent fulldecent changed the title Buy a Mac Mini M1, for testing edge cases differing across processors Use Mac for testing edge cases differing across processors Dec 24, 2024
@drinckes
Copy link
Contributor

We always knew that different architectures/implementations would give different results due to different floating point implementations, but the question is, what do we do about it?

Modifying all the OLC functions to take integers would make writing our test cases easier, but at the expense of making more work for users (because they have to remember to multiple everything by 1e7 for example). The floating point precision issues remain, they just become not our problems, but that's not really a solution.

One possible solution is to migrate the C (and maybe C++) implementations to using an arbitrary precision library such as MPFR. I have created #669 to discuss, and in the meanwhile I propose closing this PR without merging?

@fulldecent
Copy link
Contributor Author

Regardless of which solution is picked, we may want to keep the Mac test target for the long-term. It identified a math issue in the implementations and it may identify build issues in the future or provide other useful feedback.

@drinckes
Copy link
Contributor

I resolved at least one test case failure on ARM64 by changing the code from:

long long int lat = kLatMaxDegrees * 2.5e7;
lat += lat * 2.5e7;

to

long long int lat = kLatMaxDegrees * 2.5e7;
long long int x = lat * 2.5e7;
lat += x;

which really should be the same. You'd think. Well it just goes to show. :-)

@drinckes
Copy link
Contributor

More information: This is due to the compiler using different operations, as described in https://stackoverflow.com/questions/51124436/strange-issue-with-floating-point-accuracy-on-arm64.

I was able to reproduce using https://godbolt.org/z/sP5TMqqaY, code:

/*
  Example illustrating floating point differences on armv8-a.
  https://github.com/google/open-location-code/pull/665

  The compiler clang 13.0.0 succeeds, clang 14.0.0 fails unless the -ffast-math flag is used.
*/

#include <stdio.h>

long long int lng(double num) {
    long long int result = 1474560000;
    result += num * 8.192e6;
    return result;
}

int main() {
    if (lng(-159.936) == 164364288) {
        return 0; // success
    } else {
        return 1; // failure
    }
}

Compiling with armv8-a clang 13.0.0 works, armv8-a clang 14.0.0 fails unless the flag -ffast-math is added. (This isn't a solution I want to rely on though because who wants to play whack-a-mole with compiler flags?)

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.

2 participants