Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Fix a COMDouble::Round() issue#12210

Merged
janvorli merged 3 commits into
dotnet:masterfrom
hanblee:math-round
Jun 14, 2017
Merged

Fix a COMDouble::Round() issue#12210
janvorli merged 3 commits into
dotnet:masterfrom
hanblee:math-round

Conversation

@hanblee
Copy link
Copy Markdown

@hanblee hanblee commented Jun 9, 2017

@hanblee
Copy link
Copy Markdown
Author

hanblee commented Jun 9, 2017

@tannergooding PTAL

@hanblee
Copy link
Copy Markdown
Author

hanblee commented Jun 9, 2017

@tannergooding BTW, if you are not the right person to review this, it would be great if you would please pull in the right person(s). Thanks!

@tannergooding
Copy link
Copy Markdown
Member

Taking a look now.

@tannergooding
Copy link
Copy Markdown
Member

It is important to note that the MSDN page for Math.Round(double) states:

Rounds a double-precision floating-point value to the nearest integral value.

However, the actual behavior, which is on the general Math.Round page is:

Rounds a double-precision floating-point value to the nearest integer, and rounds midpoint values to the nearest even number

The secondary portion (rounding midpoint values to the nearest even number) is important for just this scenario and the exact behavior taken here.

Additionally, as an FYI, the original (unmodified) code from Desktop was hanblee@08786f2#diff-884cab80438864e5dc66c7e4c1a3c723L467, which is identical behavior to implementation currently checked in.

When completely "unrolled" the current implementation is doing

(floor(x + 0.5) == (x + 0.5)) && (fmod((x + 0.5), 2.0) != 0)

and the newly proposed implementation is doing:

(x == (floor(x) + 0.5)) && (fmod(floor(x + 0.5), 2.0) != 0)

The previous algorithm would end up comparing "precisely 1" vs "precisely 1".
The new algorithm ends up comparing 0.5 vs 0.50000000000000011, which is what allows this fix.

This is definitely an improvement as it should only do the midpoint rounding calculation when x is exactly midpoint (as best as can be represented by IEEE double-precision floating-point values, anyways).

@tannergooding
Copy link
Copy Markdown
Member

It would be great if you could add a couple tests covering this scenario (CoreFX is the primary repo for managed tests, but there are some tests in CoreCLR as well: https://github.com/dotnet/coreclr/tree/32f0f9721afb584b4a14d69135bea7ddc129f755/tests/src/CoreMangLib/cti/system/math)

@tannergooding
Copy link
Copy Markdown
Member

FYI. @janvorli, @AndyAyersMS, @jkotas who have reviewed these types of changes previously.

@hanblee
Copy link
Copy Markdown
Author

hanblee commented Jun 10, 2017

It would be great if you could add a couple tests covering this scenario

Good point. Will do.


if (Math.Round(doubleVal) != expectedVal)
{
Console.WriteLine("actual value = " + Math.Round(doubleVal));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you use the G17 format specifier when printing here and below?

> double doubleVal = 0.50000000000000011102230246251565404236316680908203;
> Console.WriteLine(doubleVal)
0.5
> Console.WriteLine("{0:G17}", doubleVal)
0.50000000000000011

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@tannergooding
Copy link
Copy Markdown
Member

Thanks! This will need someone from the CoreCLR team to give final approval before it can be merged.

@hanblee
Copy link
Copy Markdown
Author

hanblee commented Jun 12, 2017

Got it!

@hanblee
Copy link
Copy Markdown
Author

hanblee commented Jun 14, 2017

@janvorli PTAL

Copy link
Copy Markdown
Member

@janvorli janvorli left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Incorrect result from Math.Round method

5 participants