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

Math.Round(value, digits, mode) does not return the correct result #1643

Open
Tracked by #47244
tannergooding opened this issue Apr 18, 2019 · 13 comments
Open
Tracked by #47244
Assignees
Labels
area-System.Numerics breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. bug Cost:M Work that requires one engineer up to 2 weeks help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@tannergooding
Copy link
Member

The Math.Round(value, digits, mode) code returns a close, but not correct result most of the time.

The root problem is that the algorithm tries to follow normal mathematical rules to produce the result. However, due to the variable precision in IEEE floating-point numbers, this ultimately causes it to produce a close, but generally incorrect result most of the time and causes it to not accept inputs above a certain range. For example, you can't round double.MaxValue and the larger the input is, the more inaccurate the rounding will be.

These functions should be rewritten to be IEEE compliant and to accept any valid finite floating-point input.

@tannergooding tannergooding self-assigned this Apr 18, 2019
@tannergooding
Copy link
Member Author

tannergooding commented Apr 18, 2019

An example of where this fails is: Math.Round(655.924999999999954525264911353588104248046875, 2, MidpointRounding.AwayFromZero);

The nearest representable value with 2 digits is: 655.91999999999996 (which is the nearest representable value for 655.92 printed with 17 digits of precision), which is 0.004999999999994525264911353588104248046875 away. The next highest representable value with two digits is 655.92999999999995 (which is the nearest representable value for 655.93 printed with 17 digits of precision), which is 0.004999999999995474735088646411895751953125 away.

For this case, the Math.Round code is scaling the input by 100 and getting exactly 65592.5, this ends up losing thinking the original value is a midpoint value (it is not, the actual value is just below a midpoint) and rounding away from zero.

@tannergooding tannergooding transferred this issue from dotnet/corefx Jan 12, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the untriaged New issue has not been triaged by the area owner label Jan 12, 2020
@danmoseley danmoseley added the help wanted [up-for-grabs] Good issue for external contributors label Feb 25, 2020
@tannergooding tannergooding added bug and removed untriaged New issue has not been triaged by the area owner labels Mar 5, 2020
@tannergooding
Copy link
Member Author

Math.Truncate works exactly as intended. It is not a rounding function and instead just truncates all bits, always. This means 3.9 is by design returning 3.

I need to dig up the notes on MidpointRounding.ToNegativeInfinity, ToPositiveInfinity, and ToZero` as I remember there being discussion around there behavior differing.

@tannergooding tannergooding added the breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. label Jun 19, 2020
@tannergooding tannergooding added this to the Future milestone Jun 23, 2020
@tannergooding tannergooding added the Cost:M Work that requires one engineer up to 2 weeks label Jan 15, 2021
MichalStrehovsky pushed a commit to MichalStrehovsky/runtime that referenced this issue Dec 9, 2021
@jeffhandley jeffhandley modified the milestones: Future, 7.0.0 Feb 6, 2022
@ehosaini
Copy link

For Math.Round(1111_1111_1111_1111.5, 1, MidpointRounding.AwayFromZero) we get 1111111111111111.6 which we feel might be related to this issue.

@dakersnar dakersnar modified the milestones: 7.0.0, 8.0.0 Aug 8, 2022
@dakersnar dakersnar modified the milestones: 8.0.0, Future Sep 29, 2022
@dakersnar
Copy link
Contributor

We would like to fix this at some point, but it is a non-trivial issue, so this is not guaranteed to happen for .NET 8. Moved to Future for now.

@DasNaughtie
Copy link

Hi Guys, hope all's well! Curious to know when you're going to get round to this issue? I've recently moved an application to Net 8 and ran into this.

@tannergooding
Copy link
Member Author

I've recently moved an application to Net 8 and ran into this.

@DasNaughtie There's been no change in behavior since .NET Framework, this is tracking making an eventual breaking change to the behavior to fix it to do the correct thing.

Could you elaborate on what it is you're seeing?

@DasNaughtie
Copy link

DasNaughtie commented Jan 29, 2025

After spending a number of hours trying to get to the bottom of this the problem seems to exist with EF core - I'm running a linq query that is correct in VS 2022 but when I execute it profiler is showing me completely different values! 0.005 is being converted to 1 and 0.0004 is being converted to 0.0004000000000000000191686944095437183932517655

@tannergooding
Copy link
Member Author

You're observing the raw underlying value in this case.

float and double are binary-based floating-point numbers and so can only exactly represent values that are multiples of some power of 2 (with the multiple changing every power of 2, but with the root multiplier being T.Epsilon). Anything that isn't representable rounds to the nearest representable result.

0.0004 is such a case (as it is not a multiple of double.Epsilon) and so it rounds to the nearest representable which is 0.0004000000000000000191686944095437183932517655193805694580078125. double.ToString() then defaults to showing the shortest roundtrippable string (that is the string with the least number of significant digits that produces the same double), which is 0.0004 in this case, but debuggers, profilers, and other scenarios may show the exact represented value instead.

There was notably a bug fix introduced in .NET Core 3.0: https://devblogs.microsoft.com/dotnet/floating-point-parsing-and-formatting-improvements-in-net-core-3-0/, which is that we changed to ensuring that double.ToString() would produce such a roundtrippable value by default; as you have cases like double.Parse(Math.PI.ToString()) != Math.PI on .NET Framework. But that doesn't look to be what you're hitting here.

Because the literal you typed is not necessarily the exact underlying value, this can lead to some weird rounding edge cases. For example, 0.0000005 looks like a midpoint value, but it is actually 0.0000004999999999999999773740559129431293428069693618454039096832275390625 and so is rather just under and should not be treated as a midpoint.

@DasNaughtie
Copy link

DasNaughtie commented Jan 29, 2025

So in summary MS has taken something that worked perfectly fine and screwed it up to the point where I have to use a profiler to see the results of my unit tests because the UI doesn't represent the truth? How do you get away with this? It's taken months of work to get this solution and the multiple others into .Net only to find the ORM doesn't support the basic querying?

This is the Linq statement that is now broken:

var result = _context.Holding.Where( h => (h.ClientRef == holding.ClientRef) && (h.SecurityRef == holding.SecurityRef) && (!holding.Expense.HasValue || h.Expense == holding.Expense) && (!holding.Shares.HasValue || h.Shares == holding.Shares) && (!holding.AvExpense.HasValue || h.AvExpense == holding.AvExpense) && (!holding.Proceeds.HasValue || h.Proceeds == holding.Proceeds) && (!holding.Valuation.HasValue || h.Valuation == holding.Valuation) && (!holding.Truncation.HasValue || h.Truncation == holding.Truncation) && (!holding.Indexation.HasValue || h.Indexation == holding.Indexation) && (!holding.QEExpense.HasValue || h.QEExpense == holding.QEExpense) && (!holding.QEProceeds.HasValue || h.QEProceeds == holding.QEProceeds) && (!holding.Price.HasValue || h.Price == holding.Price));

That is as basic as it gets and EF Core doesn't support this scenario because of a float or double?

@tannergooding
Copy link
Member Author

So in summary MS has taken something that worked perfectly fine and screwed it up to the point where I have to use a profiler to see the results of my unit tests because the UI doesn't represent the truth

Nothing has changed, the behavior of Math.Round is the same today as it was in .NET Framework. As such, there is likely some other cause of the break in your code that is not related to Math.Round.

This issue is tracking a planned future break to ensure that correct results are computed as there are known cases that they have never been correct, and thus ensuring Math.Round is behaving correctly.

As for how float/double work, this is how IEEE 754 floating-point numbers work in all languages. Whether it is C, C++, Java, Swift, Rust, Go, Javascript, C#, VB, F#, etc. It is not unique to .NET, it is not unique to Microsoft, it is simply part of programming with such numbers. If you are doing something like working with currency, you should likely be using a base-10 floating-point type, such as decimal, instead. decimal still has concerns around rounding non-representable values (for example, ((1.0m / 3.0m) * 3.0m) != 1.0m), but since it is base-10 it works closer to how most people think about numbers and a given literal will typically be exactly representable.

@weltkante
Copy link
Contributor

weltkante commented Jan 29, 2025

As for how float/double work, this is how IEEE 754 floating-point numbers work in all languages

true for whats being discussed, but it looks like the discussion is passing over whats broken in the first place

0.005 is being converted to 1

thats probably whats causing issues, and its unlikely to be related to this issue or floating point representation at all

further investigation should probably focus at what happens between EF Core and the SQL statement generation

@tannergooding
Copy link
Member Author

true for whats being discussed, but it looks like the discussion is passing over whats broken in the first place

That's why I said "As such, there is likely some other cause of the break in your code that is not related to Math.Round.".

That is, there is likely something else broken in the code and not enough context to help root cause the problem. It is likely in how Expense is being set, given that it is decimal? (not double) and 0.005m is exactly representable as decimal.

@DasNaughtie
Copy link

DasNaughtie commented Jan 29, 2025

I don't know what else to say. I accept this is nothing to do with Math.Round but find it difficult to see an issue with the code.

I'll park this here and update the other issue I've created. Thanks for your patience.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-System.Numerics breaking-change Issue or PR that represents a breaking API or functional change over a prerelease. bug Cost:M Work that requires one engineer up to 2 weeks help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

No branches or pull requests

8 participants