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

Calculator output losing precision #18574

Closed
1 task
dakersnar opened this issue Jun 1, 2022 · 30 comments
Closed
1 task

Calculator output losing precision #18574

dakersnar opened this issue Jun 1, 2022 · 30 comments
Labels
External Dependency This bug or feature isn't resolved, but it's following an external work item. Issue-Bug Something isn't working Product-PowerToys Run Improved app launch PT Run (Win+R) Window Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing. Run-Plugin Things that relate with PowerToys Run's plugin interface Status-Blocked We can't make progress due to a dependency or issue

Comments

@dakersnar
Copy link

Microsoft PowerToys version

0.57.0

Running as admin

  • Yes

Area(s) with issue?

PowerToys Run

Steps to reproduce

Type 9007199254740991 * 2^43 into the PowerToys Run bar.

✔️ Expected Behavior

8cfca87a-4d86-438b-8ea6-10002031cfed

❌ Actual Behavior

adf59820-5bd6-4062-942c-3a9dd8ef9fc5

Other Software

No response

@dakersnar dakersnar added Issue-Bug Something isn't working Needs-Triage For issues raised to be triaged and prioritized by internal Microsoft teams labels Jun 1, 2022
@davidegiacometti davidegiacometti added Product-PowerToys Run Improved app launch PT Run (Win+R) Window Run-Plugin Things that relate with PowerToys Run's plugin interface labels Jun 1, 2022
@htcfreek
Copy link
Collaborator

htcfreek commented Jun 2, 2022

@dakersnar
Copy link
Author

@htcfreek Did you mean to link to this line? image

@htcfreek
Copy link
Collaborator

htcfreek commented Jun 2, 2022

@htcfreek Did you mean to link to this line? image

I think it has moved after merging the other PR today.

@dakersnar
Copy link
Author

Ahh. Which line were you intended to point to?

@htcfreek
Copy link
Collaborator

htcfreek commented Jun 2, 2022

Ahh. Which line were you intended to point to?

65

@dakersnar
Copy link
Author

Oh wow. I know what's causing this. It's a double to decimal conversion bug in .NET. dotnet/runtime#68042 (comment).

Believe it or not, the reason I was using the calculator with those inputs is because I was working on fixing this exact bug. I should have noticed that the calculator was displaying a suspiciously familiar output.

Long story short, this issue should be fixed soon once I fix that bug. In the meantime, I'll have to use a calculator not built on .NET 😄.

@htcfreek
Copy link
Collaborator

htcfreek commented Jun 2, 2022

@dakersnar
Is it correct that you working on a fix for both problems?

  • loosing precision
  • overflow which cause the result to be incorrett on mqny decimal digits

@dakersnar
Copy link
Author

@htcfreek Just the first. As Tanner mentions in that comment, the overflow "bug" brought up by the original post is actually expected. The precision loss, however, is a bug.

@htcfreek
Copy link
Collaborator

htcfreek commented Jun 2, 2022

@htcfreek Just the first. As Tanner mentions in that comment, the overflow "bug" brought up by the original post is actually expected. The precision loss, however, is a bug.

@dakersnar
You misunderstood me. I don't men that an exception is thrown. I mean this behavior: #9309 (comment)

@htcfreek
Copy link
Collaborator

htcfreek commented Jun 2, 2022

@crutkas , @jaimecbernardo
I deduped the other issue #9309 as we have the external reference to .net issue here. Can you mark this here as tracking-external and blocked
please.

@dakersnar
Copy link
Author

@htcfreek I was not aware of that issue. In the example you linked, do you happen to know the exact double that is being passed into the decimal converter? If the output of the converter is inaccurate, and we can reproduce it, I can add it as a unit test to ensure I fix it during my refactor.

@htcfreek
Copy link
Collaborator

htcfreek commented Jun 2, 2022

@htcfreek I was not aware of that issue. In the example you linked, do you happen to know the exact double that is being passed into the decimal converter? If the output of the converter is inaccurate, and we can reproduce it, I can add it as a unit test to ensure I fix it during my refactor.

@dakersnar
You can reproduce by input into pt run: =0.5555555555555555555555555555555555555555555555555555

I expected that without any calculation that input equals the output. But there comes one decimal digit where the output starts to change. Please test it number per number. Not sure how many you need.

@dakersnar
Copy link
Author

@htcfreek I am able to reproduce that in PT, but not in C# with a simple conversion. https://dotnetfiddle.net/cB36u7 (Notably, I don't include the "cultureInfo" argument that the PT code does, although I don't know if that changes things.)

Looking at the output of the unit test I wrote, would you agree that the overflow bug is not specifically caused by double to decimal conversion? Please let me know if you find a flaw in my thinking.

Interestingly, it looks like there is some precision loss with this test, so while I can't reproduce the overflow bug, this does provide another test case for my original bug.

image

@htcfreek
Copy link
Collaborator

htcfreek commented Jun 2, 2022

@htcfreek I am able to reproduce that in PT, but not in C# with a simple conversion. https://dotnetfiddle.net/cB36u7 (Notably, I don't include the "cultureInfo" argument that the PT code does, although I don't know if that changes things.)

Looking at the output of the unit test I wrote, would you agree that the overflow bug is not specifically caused by double to decimal conversion? Please let me know if you find a flaw in my thinking.

Interestingly, it looks like there is some precision loss with this test, so while I can't reproduce the overflow bug, this does provide another test case for my original bug.

image

@dakersnar
Can please do a quick test for me: What's the result of decimal.TryParse("0.5555555555555555555555555555555555555555555555555555")

can't do it on my mobile

@dakersnar
Copy link
Author

dakersnar commented Jun 2, 2022

@htcfreek
image

We have more precision here, which backs up my thought that the conversion precision bug affects this input.

@htcfreek
Copy link
Collaborator

htcfreek commented Jun 2, 2022

@dakersnar
I think I have to debug code in detail. There are two options:

  1. The mages library has a bug.
  2. The number translation and input validation makes the result wrong.

@dakersnar
Copy link
Author

Sounds good. And like I said, I'll keep you updated when I fix the precision bug.

@htcfreek
Copy link
Collaborator

htcfreek commented Jun 2, 2022

@htcfreek
image

We have more precision here, which backs up my thought that the conversion precision bug affects this input.

Aaah. The input in the test code here becomes incorrect on double.ToString()

@dakersnar
Copy link
Author

Does it? I want to point out that double x in my screenshot is being initialized with a different input than decimal y, because line 7 is left over from my first example.

@htcfreek
Copy link
Collaborator

htcfreek commented Jun 2, 2022

Does it? I want to point out that double x in my screenshot is being initialized with a different input than decimal y, because line 7 is left over from my first example.

I thought the output of x has to stop at the last 5 or at least fill with zero. Wonder where all these digits come from.

Or is there something mathematics that I don't have in mind?

@dakersnar
Copy link
Author

dakersnar commented Jun 2, 2022

Floating-point numbers are inherently inaccurate, so in this case, that is the closest memory representation of the input we give it.

You can see in this online calculator that it tells us that the "Most accurate representation" of our input is similar to what we see in the C# output. https://www.binaryconvert.com/result_double.html?decimal=048046053053053053053053053053053053053053053053053053053053053

image

@jaimecbernardo jaimecbernardo added External Dependency This bug or feature isn't resolved, but it's following an external work item. Status-Blocked We can't make progress due to a dependency or issue and removed Needs-Triage For issues raised to be triaged and prioritized by internal Microsoft teams labels Jun 2, 2022
@htcfreek
Copy link
Collaborator

htcfreek commented Jun 2, 2022

@jaimecbernardo , @dakersnar
The bug in PT comes from mage core library. It happens with decimal and int numbers.

@htcfreek
Copy link
Collaborator

htcfreek commented Jun 2, 2022

@jaimecbernardo , @dakersnar The bug in PT comes from mage core library. It happens with decimal and int numbers.

The following is happening:
image

image

@jaimecbernardo
Should we open an external issue on mage core project? If yes, can you do that please.

@Jay-o-Way
Copy link
Collaborator

Looks like mages has a number of problems. I think #2265 would solve this (?)

@crutkas
Copy link
Member

crutkas commented Jun 3, 2022

I think moving to calc engine would help us centralize issues.

Mage's is open source as well so we could do the PR against Mages but 100% we should get their opinion on the issue just like we like feedback

@crutkas
Copy link
Member

crutkas commented Jun 3, 2022

end of day, this is a /dup #2265 as that is where we've been funneling these against

@ghost
Copy link

ghost commented Jun 3, 2022

Hi! We've identified this issue as a duplicate of another one that already exists on this Issue Tracker. This specific instance is being closed in favor of tracking the concern over on the referenced thread. Thanks for your report!

@ghost ghost closed this as completed Jun 3, 2022
@ghost ghost added the Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing. label Jun 3, 2022
@htcfreek
Copy link
Collaborator

htcfreek commented Jun 3, 2022

@crutkas
I have opened an issue on the mages repository that this can be analyzed and fixed. This doesn't mean that we won't move to calc engine.

Is there any information about the state of calc engine and how long it need to get ready for our use case?

@htcfreek
Copy link
Collaborator

htcfreek commented Jun 16, 2022

@crutkas

I have opened an issue on the mages repository that this can be analyzed and fixed. This doesn't mean that we won't move to calc engine.

The bug in mages is fixed (FlorianRappl/Mages#110) and the fix will be available in v2.0.1 of mages. Can we then update the engine reference and test this?

@jaimecbernardo
Copy link
Collaborator

Hi @htcfreek ,
I've published an unsigned debug build here, what contains Mages 2.0.1:
https://github.com/jaimecbernardo/PowerToys/releases/tag/test-issue-18574-mages-2.0.1

I think the presented calculations in this issue are still not working. (might be something on our end, though)

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
External Dependency This bug or feature isn't resolved, but it's following an external work item. Issue-Bug Something isn't working Product-PowerToys Run Improved app launch PT Run (Win+R) Window Resolution-Duplicate There's another issue on the tracker that's pretty much the same thing. Run-Plugin Things that relate with PowerToys Run's plugin interface Status-Blocked We can't make progress due to a dependency or issue
Projects
None yet
Development

No branches or pull requests

6 participants