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

Math.Cos(double.MaxValue) returns different values on Windows and other platforms #4318

Closed
mellinoe opened this Issue Apr 13, 2016 · 7 comments

Comments

Projects
None yet
6 participants
@mellinoe
Contributor

mellinoe commented Apr 13, 2016

Scenario:

Console.WriteLine(Math.Cos(double.MaxValue));

Expected behavior

double.MaxValue is returned according to the .NET Framework documentation:

Acceptable values of d range from approximately -9223372036854775295 to approximately 9223372036854775295. For values outside this range, the Cos method returns d unchanged rather than throwing an exception.

Actual behavior

on Windows, double.MaxValue is returned. On my Ubuntu test machine, -0.999987689... is returned.

This may be an acceptable difference in behavior, as long as we've acknowledged it. It seems like we could easily match this behavior by doing some parameter boundary checks, but it may not be worth it. I'm assuming this behavior difference is coming from the native cosine functions we are using.

@gkhanna79

This comment has been minimized.

Show comment
Hide comment
@gkhanna79
Member

gkhanna79 commented Apr 14, 2016

@janvorli PTAL

@janvorli

This comment has been minimized.

Show comment
Hide comment
@janvorli

janvorli Apr 14, 2016

Member

Here is top of the call stack of the Math.Cos invocation on Linux:

  * frame #0: 0x00007ffff73cacf0 libm.so.6`???
    frame #1: 0x00007ffff6624df1 libcoreclr.so`COMDouble::Cos(d=1.7976931348623157E+308) + 49 at floatnative.cpp:147

and here is the source:

FCIMPL1_V(double, COMDouble::Cos, double d) 
    FCALL_CONTRACT;

    return (double) cos(d);
FCIMPLEND

So the behavior clearly reflects the C++ math library cos behavior on each platform.
I would prefer keeping it as is instead of adding the overhead of simulating the windows behavior.

Member

janvorli commented Apr 14, 2016

Here is top of the call stack of the Math.Cos invocation on Linux:

  * frame #0: 0x00007ffff73cacf0 libm.so.6`???
    frame #1: 0x00007ffff6624df1 libcoreclr.so`COMDouble::Cos(d=1.7976931348623157E+308) + 49 at floatnative.cpp:147

and here is the source:

FCIMPL1_V(double, COMDouble::Cos, double d) 
    FCALL_CONTRACT;

    return (double) cos(d);
FCIMPLEND

So the behavior clearly reflects the C++ math library cos behavior on each platform.
I would prefer keeping it as is instead of adding the overhead of simulating the windows behavior.

@mellinoe

This comment has been minimized.

Show comment
Hide comment
@mellinoe

mellinoe Apr 14, 2016

Contributor

Yes, this is what I expected to be the cause, but it's good to have confirmation. I am fine with this behavior difference as well, but I wanted to call it out to make sure it had seen some scrutiny other than my own. I'm not sure if we have a list of such low-level behavioral differences, but if we do we should document this one.

Contributor

mellinoe commented Apr 14, 2016

Yes, this is what I expected to be the cause, but it's good to have confirmation. I am fine with this behavior difference as well, but I wanted to call it out to make sure it had seen some scrutiny other than my own. I'm not sure if we have a list of such low-level behavioral differences, but if we do we should document this one.

@janvorli

This comment has been minimized.

Show comment
Hide comment
@janvorli

janvorli Apr 14, 2016

Member

I am not aware of such a list. @richlander do we have some doc where a list of platform specific differences as @mellinoe has suggested would fit?

Member

janvorli commented Apr 14, 2016

I am not aware of such a list. @richlander do we have some doc where a list of platform specific differences as @mellinoe has suggested would fit?

@sergiy-k sergiy-k modified the milestones: 1.1.0, 1.0.0-rtm May 5, 2016

@gkhanna79

This comment has been minimized.

Show comment
Hide comment
@gkhanna79

gkhanna79 Oct 13, 2016

Member

@leecow @blackdwarf Can you please add this to the documented difference list we prepare for each release?

Member

gkhanna79 commented Oct 13, 2016

@leecow @blackdwarf Can you please add this to the documented difference list we prepare for each release?

@janvorli

This comment has been minimized.

Show comment
Hide comment
@janvorli

janvorli Oct 25, 2016

Member

I can see a reference to this issue was added to https://github.com/dotnet/core/blob/master/release-notes/1.1/1.1.0-preview1-known-issues.md, so I am closing this issue.

Member

janvorli commented Oct 25, 2016

I can see a reference to this issue was added to https://github.com/dotnet/core/blob/master/release-notes/1.1/1.1.0-preview1-known-issues.md, so I am closing this issue.

@janvorli janvorli closed this Oct 25, 2016

@omajid

This comment has been minimized.

Show comment
Hide comment
@omajid

omajid Nov 18, 2016

Member

Would adding a math library that has matching results on all platforms be worth it? Java has a StrictMath (separate from the regular Math) class that requires implementations to return values that match published algorithms.

Member

omajid commented Nov 18, 2016

Would adding a math library that has matching results on all platforms be worth it? Java has a StrictMath (separate from the regular Math) class that requires implementations to return values that match published algorithms.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment