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

Issue 38160 - MidpointRounding is buggy #38563

Closed
wants to merge 4 commits into from
Closed

Issue 38160 - MidpointRounding is buggy #38563

wants to merge 4 commits into from

Conversation

FrantzUml
Copy link

@FrantzUml FrantzUml commented Jun 29, 2020

From the [https://github.com//issues/38160] issue

USAGE
The System.Math.Round and System.MathF.Round is buggy when using MidpointRounding and the algorithm may overflow when using digits.
The MidpointRounding counts some directed round (and not rounding to nearest integer), for example : the MidpointRounding.ToZero description says : "when a number is halfway between two others......." but in reality is a directed rounding (and not HALF).

Proposed API
To avoid breaking changes, a new enum for the rounding mode is proposed, the rounding functions using MidpointRounding are marked as obsolete (except for MidpointRounding.ToEven that is correct), and an implementation with the new enum is proposed.

public enum RoundingMode
{
	/// <summary>
	/// Directed round down (same as Floor).
	/// </summary>
	/// <remarks>
	/// <code>Assert.AreEqual(-2D, -1.8D);</code>
	/// <code>Assert.AreEqual(-2D, -1.5D);</code>
	/// <code>Assert.AreEqual(-2D, -1.2D);</code>
	/// <code>Assert.AreEqual(1D, +1.2D);</code>
	/// <code>Assert.AreEqual(1D, +1.5D);</code>
	/// <code>Assert.AreEqual(1D, +1.8D);</code>
	/// </remarks>
	Down = 3,

	/// <summary>
	/// Directed round up (same as Ceiling).
	/// </summary>
	/// <remarks>
	/// <code>Assert.AreEqual(-1D, -1.8D);</code>
	/// <code>Assert.AreEqual(-1D, -1.5D);</code>
	/// <code>Assert.AreEqual(-1D, -1.2D);</code>
	/// <code>Assert.AreEqual(2D, +1.2D);</code>
	/// <code>Assert.AreEqual(2D, +1.5D);</code>
	/// <code>Assert.AreEqual(2D, +1.8D);</code>
	/// </remarks>
	Up = 4,

	/// <summary>
	/// Directed round towards zero (same as Truncate).
	/// </summary>
	/// <remarks>
	/// <code>Assert.AreEqual(-1D, -1.8D);</code>
	/// <code>Assert.AreEqual(-1D, -1.5D);</code>
	/// <code>Assert.AreEqual(-1D, -1.2D);</code>
	/// <code>Assert.AreEqual(1D, +1.2D);</code>
	/// <code>Assert.AreEqual(1D, +1.5D);</code>
	/// <code>Assert.AreEqual(1D, +1.8D);</code>
	/// </remarks>
	TowardsZero = 2,

	/// <summary>
	/// Directed round away from zero
	/// </summary>
	/// <remarks>
	/// <code>Assert.AreEqual(-2D, -1.8D);</code>
	/// <code>Assert.AreEqual(-2D, -1.5D);</code>
	/// <code>Assert.AreEqual(-2D, -1.2D);</code>
	/// <code>Assert.AreEqual(2D, +1.2D);</code>
	/// <code>Assert.AreEqual(2D, +1.5D);</code>
	/// <code>Assert.AreEqual(2D, +1.8D);</code>
	/// </remarks>
	AwayZero = 5,

	/// <summary>
	/// Round to nearest integer and down when the number is halfway between two others.
	/// </summary>
	/// <remarks>
	/// <code>Assert.AreEqual(-2D, -1.8D);</code>
	/// <code>Assert.AreEqual(-2D, -1.5D);</code>
	/// <code>Assert.AreEqual(-1D, -1.2D);</code>
	/// <code>Assert.AreEqual(1D, +1.2D);</code>
	/// <code>Assert.AreEqual(1D, +1.5D);</code>
	/// <code>Assert.AreEqual(2D, +1.8D);</code>
	/// </remarks>
	HalfDown = 6,

	/// <summary>
	/// Round to nearest integer and up when the number is halfway between two others.
	/// </summary>
	/// <remarks>
	/// <code>Assert.AreEqual(-2D, -1.8D);</code>
	/// <code>Assert.AreEqual(-1D, -1.5D);</code>
	/// <code>Assert.AreEqual(-1D, -1.2D);</code>
	/// <code>Assert.AreEqual(1D, +1.2D);</code>
	/// <code>Assert.AreEqual(2D, +1.5D);</code>
	/// <code>Assert.AreEqual(2D, +1.8D);</code>
	/// </remarks>
	HalfUp = 7,

	/// <summary>
	/// Round to nearest integer and towards zero when the number is halfway between two others.
	/// </summary>
	/// <remarks>
	/// <code>Assert.AreEqual(-2D, -1.8D);</code>
	/// <code>Assert.AreEqual(-1D, -1.5D);</code>
	/// <code>Assert.AreEqual(-1D, -1.2D);</code>
	/// <code>Assert.AreEqual(1D, +1.2D);</code>
	/// <code>Assert.AreEqual(1D, +1.5D);</code>
	/// <code>Assert.AreEqual(2D, +1.8D);</code>
	/// </remarks>
	HalfTowardsZero = 8,

	/// <summary>
	/// Round to nearest integer and away from zero when the number is halfway between two others.
	/// </summary>
	/// <remarks>
	/// <code>Assert.AreEqual(-2D, -1.8D);</code>
	/// <code>Assert.AreEqual(-2D, -1.5D);</code>
	/// <code>Assert.AreEqual(-1D, -1.2D);</code>
	/// <code>Assert.AreEqual(1D, +1.2D);</code>
	/// <code>Assert.AreEqual(2D, +1.5D);</code>
	/// <code>Assert.AreEqual(2D, +1.8D);</code>
	/// </remarks>
	HalfAwayZero = 1,

	/// <summary>
	/// Round to nearest integer and to nearest even integer when the number is halfway between two others.
	/// </summary>
	/// <remarks>
	/// <code>Assert.AreEqual(-2D, -1.8D);</code>
	/// <code>Assert.AreEqual(-2D, -1.5D);</code>
	/// <code>Assert.AreEqual(-1D, -1.2D);</code>
	/// <code>Assert.AreEqual(0D, -0.5D);</code>
	/// <code>Assert.AreEqual(0D, +0.5D);</code>
	/// <code>Assert.AreEqual(1D, +1.2D);</code>
	/// <code>Assert.AreEqual(2D, +1.5D);</code>
	/// <code>Assert.AreEqual(2D, +1.8D);</code>
	/// </remarks>
	HalfToEven = 0
}```

To avoid an overflow, just the fractional part of the value (double or float) is powered by 10^digits and not the entire value.

The MidpointRounding enum is buggy as some are "directed rounding" and not Half nearest rounding.
Replace MidpointRounding by a new RoundingMode with correct rounding.
And the algorithm with 'int digits' may overflow with 'value *= power10;'
So, the correction is to pow only the fractional part of the value.
A type of rounding that is compliant with ieee that contains 'directed rounding' and 'nearest rounding'.
Get a new RoundingMode that is IEEE compliant as MidpointRounding is buggy.
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 29, 2020
@dnfadmin
Copy link

dnfadmin commented Jun 29, 2020

CLA assistant check
All CLA requirements met.

@scalablecory
Copy link
Contributor

scalablecory commented Jun 29, 2020

@FrantzUml, thanks for your willingness to jump on this issue.

Before adding/changing APIs, we have an API review process that must be followed. Please discuss your API in #38160 to allow it to be reviewed first.

I'm going to close this PR per the review guidelines:

Pull requests against dotnet/runtime shouldn't be submitted before getting approval. Also, we don't want to get work in progress (WIP) PR's. The reason being that we want to reduce the number pending PRs so that we can focus on the work the community expects we take action on.

If you want to collaborate with other people on the design, feel free to perform the work in a branch in your own fork. If you want to track your TODOs in the description of a PR, you can always submit a PR against your own fork. Also, feel free to advertise your PR by linking it from the issue you filed against dotnet/runtime in the first step above.

If the API gets eventually marked with an "approved", please feel free to submit a new PR for the approved API.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants