Double (and Float) to Decimal Conversion Precision Improvement #72217
Conversation
This reverts commit 1c3556d.
|
I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label. |
| @@ -537,5 +561,171 @@ private static unsafe uint Dragon4(ulong mantissa, int exponent, uint mantissaHi | |||
| Debug.Assert(outputLen <= buffer.Length); | |||
| return outputLen; | |||
| } | |||
|
|
|||
| private static unsafe Dragon4State Dragon4GetState(ulong mantissa, int exponent, uint mantissaHighBitIdx, bool hasUnequalMargins, int cutoffNumber) | |||
There was a problem hiding this comment.
Note, none of this function is new, git is just confused by my refactor. This is just the first half of the existing Dragon4 code hoisted to a helper function.
| else | ||
| { | ||
| // As soon as we find a non-zero block, the rest of remainder is significant | ||
| break; | ||
| } |
There was a problem hiding this comment.
This change has its own separate PR here: #72115
|
Tagging subscribers to this area: @dotnet/area-system-numerics Issue DetailsFixes #72135, microsoft/PowerToys#18574 Blocked by #72115 (although that change is included in this PR too) See #72135 for an explanation of the problem Summary of fixThis PR is a general refactor of the conversion code. The primary goal is to solve the reported bug, but secondarily make this conversion more efficient by reusing fast code we have written elsewhere. To achieve this, @tannergooding and I brainstormed reusing a section of the Dragon4 algorithm for double to string conversion, as printing a double already involves converting it to base 10. To do this, I split up the code for the Dragon4 algorithm into two parts and added an additional API that allows Decimal.DecCalc.cs access to the first half of Dragon4. This API is structured to return everything Decimal.DecCalc.cs needs to construct a decimal representation of the original double. TestingTo see some of the impact of the change, take a look at how much existing test data had to be updated in Problems/TODO
|
|
@dakersnar What is the status of this PR? If it's blocked or work is deferred we should change it to draft status. |
|
We determined the change was a bit too drastic to merge that late in the release cycle for .NET 7.0. We are planning to merge it into .NET 8. Should I mark it as a draft for now? |
|
@dakersnar up to you, but in general we mark PR's draft when they aren't close to mergeable, eg., work has paused, significant design issues need to be resolved, it's blocked on another change, etc. I believe validation can still be triggered on draft PR's, but e.g., we no longer report it in the weekly "stale PR's" mailers. We focus on moving along the non-draft ones. Another option when work on a PR is being deferred (not suggesting here) is to close it, as it's one click to reopen. The only side effect is that it reruns all validation when you do that, which is sometimes desirable sometimes not. |
|
This should be pretty much ready to review but converting it to draft for now as we don't plan on doing that for a bit. Thanks for the suggestion. |
ghost
closed this
|
Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it. |
ghost
locked as resolved and limited conversation to collaborators
Fixes #72135, microsoft/PowerToys#18574
Blocked by #72115 (although that change is included in this PR too)
See #72135 for an explanation of the problem
Summary of fix
This PR is a general refactor of the conversion code. The primary goal is to solve the reported bug, but secondarily make this conversion more efficient by reusing fast code we have written elsewhere. To achieve this, @tannergooding and I brainstormed reusing a section of the Dragon4 algorithm for double to string conversion, as printing a double already involves converting it to base 10.
To do this, I split up the code for the Dragon4 algorithm into two parts and added an additional API that allows Decimal.DecCalc.cs access to the first half of Dragon4. This API is structured to return everything Decimal.DecCalc.cs needs to construct a decimal representation of the original double.
Testing
To see some of the impact of the change, take a look at how much existing test data had to be updated in
DecimalTests.cs. Most of the expected results were wrong but the tests were passing because (I assume) they were built to match the output of the incorrect converter. To ensure that I wasn't relying on my conversion code as the "source of truth" to test itself, I generated the expected results using the decimal parser.Problems/TODO
decimal.GetBits(decimal.Parse(doubleValue.ToString("G99"), System.Globalization.NumberStyles.Float)by removing trailing zeros for non-zero values. However, in some cases, the parser ends up retaining some trailing zeros, and there is a mismatch. In these cases, the output of my converter is still correct, it is just not completely consistent with what the parser will generate.