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

Function fmod returns different values than C++ when mixing signs #983

Open
bcliden opened this issue Nov 18, 2024 · 1 comment
Open

Function fmod returns different values than C++ when mixing signs #983

bcliden opened this issue Nov 18, 2024 · 1 comment

Comments

@bcliden
Copy link

bcliden commented Nov 18, 2024

Hello, I am writing some unit tests on nannou for a university course . I found that fmod has some slight differences compared to the C/C++ implementation. These specific examples are from the cppreference page on fmod.

C/C++ (tested with gcc 14.2.1 on fedora)

fmod(5.1, 3.0)   //  2.100000
fmod(-5.1, 3.0)  // -2.100000
fmod(5.1, -3.0)  //  2.100000
fmod(-5.1, -3.0) // -2.100000

nannou math::fmod

fmod(5.1, 3.0)   //  2.0999999999999996 ✅
fmod(-5.1, 3.0)  //  0.9000000000000004 ❌
fmod(5.1, -3.0)  // -0.9000000000000004 ❌
fmod(-5.1, -3.0) // -2.0999999999999996 ✅

As you can see, tests 2 and 3 are different. I believe this is due to the usage of floor (lower integer) rather than truncate (integer towards zero). The man page for fmod includes the following description:

These functions compute the floating-point remainder of dividing x by
y. The return value is x - n * y, where n is the quotient of x / y,
rounded toward zero to an integer.

So I believe using trunc() here is the correct move if we want to be close to the C implementation. Using trunc results in the expected results (see Rust Playground here)

If the project agrees, I'd like to submit a PR and unit tests for this change. Thanks!

@JoshuaBatty
Copy link
Member

Hi @bcliden. Thanks for opening the issue and for clearly showing the expected behaviour.

This seems reasonable to me. Feel free to open up a PR with this change. @mitchmindtree do you remember what you were referencing when you ported this function originally?

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

No branches or pull requests

2 participants