Add new rounding modes to System.Math, System.MathF #20815

hamish-rose · 2018-11-05T22:08:44Z

resolves dotnet/corefx#31902

Adding new rounding modes to MidpointRounding for use with Math.Round and MathF.Round.

My unit tests are over on corefx at the moment. If I understand correctly I need my changes to be mirrored there by the bot and corefx be built with my coreclr version to run tests.

See https://github.com/hamish-rose/corefx/blob/MidpointRoundingmodes%2331902/src/System.Runtime.Extensions/tests/System/Math.cs#L1469

src/System.Private.CoreLib/shared/System/Decimal.cs

tannergooding · 2018-11-05T22:15:29Z

src/System.Private.CoreLib/shared/System/Math.cs

+                {
+                    double fraction = ModF(value, &value);
+
+                    if (Sign(fraction) == -1)


This should only come into play for ties.

Decimal.DecCalc.InternalRound with Truncate mode supplied (AKA ToZero if i'm not mistaken) currently behaves like this aswell.

e.g. these scenarios all pass..

[InlineData(11.0, 11, MidpointRounding.ToZero)] [InlineData(11.4, 11, MidpointRounding.ToZero)] [InlineData(11.5, 11, MidpointRounding.ToZero)] [InlineData(11.6, 11, MidpointRounding.ToZero)] [InlineData(-11.0, -11, MidpointRounding.ToZero)] [InlineData(-11.4, -11, MidpointRounding.ToZero)] [InlineData(-11.5, -11, MidpointRounding.ToZero)] [InlineData(-11.6, -11, MidpointRounding.ToZero)] public static void Round_Decimal_Modes(decimal x, decimal expected, MidpointRounding mode) { Assert.Equal(expected, Math.Round(x, 0, mode)); Assert.Equal(expected, decimal.Round(x, 0, mode)); }

Agreed its not really midpoint rounding though, perhaps Math.Round(decimal, ..) could be implemented in Math.cs ?

Right. Sorry, I was thinking about the case where you are parsing a result.

Having comments explaining each case would be helpful. We should also just be able to use Ceiling, Floor, and Truncate (for ToPositiveInfinity, ToNegativeInfinity, and Truncate, respectively)

Right. Sorry, I was thinking about the case where you are parsing a result.

Having comments explaining each case would be helpful. We should also just be able to use Ceiling, Floor, and Truncate (for ToPositiveInfinity, ToNegativeInfinity, and Truncate, respectively)

Done, added some comments and now using Floor, Ceiling and Truncate.

tannergooding · 2018-11-05T22:24:44Z

src/System.Private.CoreLib/shared/System/Math.cs

+                        value--;
+                    }
+                }      
+                else if (mode is MidpointRounding.ToPositiveInfinity)


The result shall be the format’s floating-point number (possibly +∞) closest to and no less than the infinitely precise result.

tannergooding · 2018-11-05T22:24:58Z

src/System.Private.CoreLib/shared/System/Math.cs

-                    value = Round(value);
+                    value = Truncate(value);
+                }
+                else if (mode is MidpointRounding.ToNegativeInfinity)


The result shall be the format’s floating-point number (possibly −∞) closest to and no greater than the infinitely precise result

tannergooding · 2018-11-05T22:25:16Z

src/System.Private.CoreLib/shared/System/Math.cs


                    if (Abs(fraction) >= 0.5)
                    {
                        value += Sign(fraction);
                    }
                }
-                else
+                else if (mode is MidpointRounding.ToZero)


The result shall be the format’s floating-point number closest to and no greater in magnitude than the infinitely precise result.

tannergooding · 2018-11-05T22:25:41Z

src/System.Private.CoreLib/shared/System/Math.cs

                {
-                    var fraction = ModF(value, &value);
+                    value = Round(value);


The floating-point number nearest to the infinitely precise result shall be delivered; if the two nearest floating-point numbers bracketing an unrepresentable infinitely precise result are equally near, the one with an even least significant digit shall be delivered

tannergooding · 2018-11-05T22:25:59Z

src/System.Private.CoreLib/shared/System/Math.cs

+                }
+                else if (mode is MidpointRounding.AwayFromZero)
+                {
+                    double fraction = ModF(value, &value);


The floating-point number nearest to the infinitely precise result shall be delivered; if the two nearest floating-point numbers bracketing an unrepresentable infinitely precise result are equally near, the one with larger magnitude shall be delivered.

danmoseley · 2018-11-07T05:41:05Z

src/System.Private.CoreLib/shared/System/MathF.cs

@@ -183,18 +183,37 @@ public static unsafe float Round(float x, int digits, MidpointRounding mode)

                x *= power10;

-                if (mode == MidpointRounding.AwayFromZero)
+                if (mode is MidpointRounding.ToEven)


would a switch statement be clearer?

I think so. I opted for if else here to be consistent with existing code such as Decimal.DecCalc.InternalRound.

I've updated the implementation in Math and MathF with a switch statement

danmoseley · 2018-11-07T05:43:38Z

@hamish-rose I see you have tests in your corefx fork. What we typically do is put up the corefx PR with the tests at the same time as the coreclr PR with the implementation. Of course the build or tests fail in the corefx PR initially but it means that we can review the tests concurrently and we know before we merge the implementation that satisfcatory tests on it are ready.

hamish-rose · 2018-11-07T09:03:48Z

@hamish-rose I see you have tests in your corefx fork. What we typically do is put up the corefx PR with the tests at the same time as the coreclr PR with the implementation. Of course the build or tests fail in the corefx PR initially but it means that we can review the tests concurrently and we know before we merge the implementation that satisfcatory tests on it are ready.

@danmosemsft i've raised dotnet/corefx#33297

tannergooding · 2018-11-17T18:49:03Z

test this please

tannergooding · 2018-11-17T18:52:56Z

src/System.Private.CoreLib/shared/System/Math.cs

                {
-                    value = Round(value);
+                    // Directed rounding: Round toward zero, to nearest value above (positive numbers) or below (negative numbers)


I think the comment needs fixing, we won't be rounding above for positive numbers, but rather the nearest exact or the nearest just below (closer to zero)

tannergooding

(sorry for the delay, this fell off my radar)

This looks correct to me (at least according to the current algorithm Math.Round(int digits) is doing).

CC. @ViktorHofer, @eerhardt as it would be useful to have a second pair of eyes.

eerhardt · 2018-11-19T15:54:58Z

            DecCalc.InternalRound(ref AsMutable(ref d), (uint)scale, (DecCalc.RoundingMode)mode);

Would it make sense to get rid of DecCalc.RoundingMode completely now and just use public enum MidpointRounding instead? That way:

We don't have 2 enums using different names to mean the same thing. It would aid in code readability.
We don't accidently get the values out of sync (since the way it exists now, the values need to match up for this to work).

Refers to: src/System.Private.CoreLib/shared/System/Decimal.cs:625 in 072f5b8. [](commit_id = 072f5b8, deletion_comment = False)

eerhardt · 2018-11-19T15:59:46Z

src/System.Private.CoreLib/shared/System/Math.cs

+                }
+                else if (mode is MidpointRounding.ToNegativeInfinity)
+                {
+                    // Directed Rounding: Round down to the next value, toward −∞


I don't think we typically put non-standard symbols in source code, as some editors/viewers don't show these correctly. Can we just put negative infinity instead?

eerhardt

Thanks for the good work, @hamish-rose! Looks good.

tannergooding · 2019-02-01T03:12:13Z

@hamish-rose, sorry for the delay, this slipped off my radar.

Could you please either merge or rebase this PR against the current head of master? Once that is done (and the jobs all pass), we should be good to merge.

new modes added to enum implemented ToZero for double in Math.cs

now round inline with DecCalc internal round implementation

also replace var to make things obvious

code review feedback

add comments, update MathF with floor/ceil

- fix comments - replace ifelse with switch - remove RoundingMode enum from DecCalc

hamish-rose · 2019-02-04T18:24:30Z

@tannergooding I believe I've rebased onto master, working on the failing test now.

tannergooding · 2019-02-04T18:29:36Z

I believe I've rebased onto master, working on the failing test now.

How did you rebase? I wouldn't expect quite so many commits. Many of them look like you may have rebased a merge commit...

krwq · 2019-02-04T19:35:37Z

you can fix/restore most of the stuff with git reflog assuming you didn't remove local copy of the repository (copy the whole coreclr folder before just in case)

hamish-rose · 2019-02-04T20:19:50Z

On my local tracking branch I rebased it onto master, then when I pushed to my remote it 'merged' and now it a bit of a mess.

Sorry about that, I'll give reflog a try.

hamish-rose · 2019-02-06T00:20:09Z

@tannergooding i've fixed the rebase now. The test that is failing is a corefx inner loop test that is obsolete now since we just have one enum for rounding mode. I think it can be removed once these changes are merged and mirrored in corefx

Round_InvalidMidpointRounding_ThrowsArgumentException

tannergooding · 2019-02-11T20:35:12Z

For the failing CoreFX tests, we need to add an entry in the CoreFX.issues.json indicating it is outdated. You should just need to add the full name of the relevant test after the other Math test entries here: https://github.com/dotnet/coreclr/blob/master/tests/CoreFX/CoreFX.issues.json#L805

tannergooding · 2019-02-12T10:55:54Z

Thanks for the work here @hamish-rose

…0815) * add new rounding modes to MidpointRounding.cs new modes added to enum implemented ToZero for double in Math.cs * ToZero implementation * implement double and float rounding modes * updating rounding implementation now round inline with DecCalc internal round implementation * small bug fix also replace var to make things obvious * update implementation - floor/ceil code review feedback * review feedback add comments, update MathF with floor/ceil * code review feedback - fix comments - replace ifelse with switch - remove RoundingMode enum from DecCalc * exclude outdated corefx test Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

pentp

Unless I'm completely misunderstanding something, then this change incorrectly conflates midpoint rounding modes with truncation/ceiling/floor which apply to any rounded value, not just to midpoint!

pentp · 2019-02-12T13:43:30Z

OK, the IEEE 754 spec actually talks about directed rounding (truncate/floor/ceiling), but then this API naming is definitely wrong - it's not midpoint rounding.

…0815) * add new rounding modes to MidpointRounding.cs new modes added to enum implemented ToZero for double in Math.cs * ToZero implementation * implement double and float rounding modes * updating rounding implementation now round inline with DecCalc internal round implementation * small bug fix also replace var to make things obvious * update implementation - floor/ceil code review feedback * review feedback add comments, update MathF with floor/ceil * code review feedback - fix comments - replace ifelse with switch - remove RoundingMode enum from DecCalc * exclude outdated corefx test Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

tannergooding · 2019-02-12T15:27:17Z

@pentp, I've commented about your concerns on the original API proposal: https://github.com/dotnet/corefx/issues/31902.

We should continue the discussion there, to help keep it in one location.

…0815) * add new rounding modes to MidpointRounding.cs new modes added to enum implemented ToZero for double in Math.cs * ToZero implementation * implement double and float rounding modes * updating rounding implementation now round inline with DecCalc internal round implementation * small bug fix also replace var to make things obvious * update implementation - floor/ceil code review feedback * review feedback add comments, update MathF with floor/ceil * code review feedback - fix comments - replace ifelse with switch - remove RoundingMode enum from DecCalc * exclude outdated corefx test Signed-off-by: dotnet-bot <dotnet-bot@microsoft.com>

#33297) * add tests for new midpointrounding modes new tests added for Math.cs double Round methods * add tests for float, decimal convert to theory * Update tests tests now reflect correct rounding behavior * add tests for existing modes * code review feedback - refactor tests to use member data - move MathF tests to MathF.netcoreapp.cs - add tests for new modes with digits and special cases * move math tests to mathtests.netcoreapp.cs netcoreapp specific tests moved

…0815) * add new rounding modes to MidpointRounding.cs new modes added to enum implemented ToZero for double in Math.cs * ToZero implementation * implement double and float rounding modes * updating rounding implementation now round inline with DecCalc internal round implementation * small bug fix also replace var to make things obvious * update implementation - floor/ceil code review feedback * review feedback add comments, update MathF with floor/ceil * code review feedback - fix comments - replace ifelse with switch - remove RoundingMode enum from DecCalc * exclude outdated corefx test Commit migrated from dotnet/coreclr@3397472

tannergooding reviewed Nov 5, 2018

View reviewed changes

src/System.Private.CoreLib/shared/System/Decimal.cs Show resolved Hide resolved

tannergooding reviewed Nov 5, 2018

View reviewed changes

danmoseley reviewed Nov 7, 2018

View reviewed changes

hamish-rose mentioned this pull request Nov 7, 2018

Add tests for dotnet/coreclr#20815 - new Math and MathF rounding modes dotnet/corefx#33297

Merged

tannergooding reviewed Nov 17, 2018

View reviewed changes

tannergooding approved these changes Nov 17, 2018

View reviewed changes

eerhardt reviewed Nov 19, 2018

View reviewed changes

eerhardt approved these changes Nov 22, 2018

View reviewed changes

hamish-rose added 8 commits February 4, 2019 20:55

add new rounding modes to MidpointRounding.cs

12b221f

new modes added to enum implemented ToZero for double in Math.cs

ToZero implementation

8a0e44c

implement double and float rounding modes

f34b285

updating rounding implementation

b9aeefd

now round inline with DecCalc internal round implementation

small bug fix

ccc1a39

also replace var to make things obvious

update implementation - floor/ceil

93eb4a6

code review feedback

review feedback

27674db

add comments, update MathF with floor/ceil

code review feedback

126815c

- fix comments - replace ifelse with switch - remove RoundingMode enum from DecCalc

exclude outdated corefx test

fd576b2

tannergooding merged commit 3397472 into dotnet:master Feb 12, 2019

pentp reviewed Feb 12, 2019

View reviewed changes

hamish-rose deleted the midpointrounding-modes-#31902 branch February 12, 2019 19:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add new rounding modes to System.Math, System.MathF #20815

Add new rounding modes to System.Math, System.MathF #20815

hamish-rose commented Nov 5, 2018

tannergooding Nov 5, 2018

hamish-rose Nov 6, 2018

tannergooding Nov 6, 2018

hamish-rose Nov 6, 2018

tannergooding Nov 5, 2018

tannergooding Nov 5, 2018

tannergooding Nov 5, 2018

tannergooding Nov 5, 2018

tannergooding Nov 5, 2018

danmoseley Nov 7, 2018

hamish-rose Nov 7, 2018 •

edited

Loading

hamish-rose Nov 23, 2018

danmoseley commented Nov 7, 2018

hamish-rose commented Nov 7, 2018

tannergooding commented Nov 17, 2018

tannergooding Nov 17, 2018

tannergooding left a comment

eerhardt commented Nov 19, 2018

eerhardt Nov 19, 2018

eerhardt left a comment •

edited

Loading

tannergooding commented Feb 1, 2019

hamish-rose commented Feb 4, 2019

tannergooding commented Feb 4, 2019

krwq commented Feb 4, 2019

hamish-rose commented Feb 4, 2019

hamish-rose commented Feb 6, 2019

tannergooding commented Feb 11, 2019

tannergooding commented Feb 12, 2019

pentp left a comment

pentp commented Feb 12, 2019

tannergooding commented Feb 12, 2019

Add new rounding modes to System.Math, System.MathF #20815

Add new rounding modes to System.Math, System.MathF #20815

Conversation

hamish-rose commented Nov 5, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hamish-rose Nov 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danmoseley commented Nov 7, 2018

hamish-rose commented Nov 7, 2018

tannergooding commented Nov 17, 2018

Choose a reason for hiding this comment

tannergooding left a comment

Choose a reason for hiding this comment

eerhardt commented Nov 19, 2018

Choose a reason for hiding this comment

eerhardt left a comment • edited Loading

Choose a reason for hiding this comment

tannergooding commented Feb 1, 2019

hamish-rose commented Feb 4, 2019

tannergooding commented Feb 4, 2019

krwq commented Feb 4, 2019

hamish-rose commented Feb 4, 2019

hamish-rose commented Feb 6, 2019

tannergooding commented Feb 11, 2019

tannergooding commented Feb 12, 2019

pentp left a comment

Choose a reason for hiding this comment

pentp commented Feb 12, 2019

tannergooding commented Feb 12, 2019

hamish-rose Nov 7, 2018 •

edited

Loading

eerhardt left a comment •

edited

Loading