Fix #222: Wrong result for e.g. √(2.25) - 1.5 #222 by guihuaz · Pull Request #2424 · microsoft/calculator

guihuaz · 2025-12-07T21:37:34Z

Fixes #222, #309, #574

Description of the changes:

add a helper function _snaprat to check if a result RAT should be zero with given precision. Apply _snaprat to addrat/subrat/lograt.
The root causes of issues Wrong result for e.g. √(2.25) - 1.5 #222, Inaccurate results for trigonometric functions with exact results #309, Incorrect result for exponational and logarithmic functions with exact results #574 are the same. For an irrational number result, the approximation introduces small errors beyond precision required, which is expected. The issue was Ratpak tried to take "precision" digits from the calculation result regardless where the result came from, and it exposed digits from approximation errors. Take sqrt(2.25) - 1.5 for example. sqrt(2.25) might be approximated to 1.499...998... depending on the precision. When this number subtracted 1.5, the Ratpak got 0.000...001.... Ratpak took up to precision number of digits and converts to scientific format, which resulted -1.2566515751494548126925332972115e-47. This PR checks the magnitude difference between the result and the input operands. Since the result is too close to 47 magnitude smaller than original subtraction operands 1.499...998... and 1.5, it would be snapped to 0.
The same fix applies to lograt as well.
One further optimization could be a new sqrt function specifically checking perfect square first before using Taylor series approximation.

How changes were validated:

Add 1 test case in CalculatorManagerTestStandard to verify addrat.
Add 2 test cases in CalculatorManagerTestScientific to verify addrat and lograt
Manual test the cases in the linked issues.
Manual test log case: log(sqrt(2.25)/1.5)

guihuaz · 2025-12-07T22:51:03Z

@guihuaz please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
@microsoft-github-policy-service agree [company="{your company}"]
Options:

(default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
(when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"
Contributor License Agreement

@microsoft-github-policy-service agree

guihuaz · 2025-12-20T07:13:00Z

@tian-lt, are you the maintainer of this repo? Could you please take a look at the PR?

tian-lt · 2025-12-23T02:00:46Z

@tian-lt, are you the maintainer of this repo? Could you please take a look at the PR?

@guihuaz Sure. Thank you for proposing the fix.
Let me trigger a CI run for this PR.
I'll review the changes and get back to you later.

Copilot

Pull request overview

This PR fixes issues #222, #309, and #574 where calculations involving irrational number approximations (like √2.25 - 1.5) produced tiny non-zero results instead of zero due to approximation errors in the Ratpak calculation engine.

Key Changes:

Introduced _snaprat helper function to detect and snap results to zero when they are magnitudes smaller than input operands beyond precision threshold
Refactored addrat, subrat, and lograt to have internal versions (_addrat, _subrat, _lograt) without snapping, with public versions applying snapping for user-facing results
Updated all internal Ratpack function calls to use the underscore-prefixed versions to avoid unnecessary snapping during intermediate calculations

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
src/CalcManager/Ratpack/rat.cpp	Implements the core fix: adds `_snaprat` function and refactors `addrat`/`subrat` to include internal versions without snapping
src/CalcManager/Ratpack/exp.cpp	Refactors `lograt` into internal `_lograt` and `__lograt` versions, with public `lograt` applying snapping; updates internal calls to use `_subrat`/`_addrat`
src/CalcManager/Ratpack/ratpak.h	Adds function declarations for `_addrat`, `_subrat`, `_lograt`, `_snaprat`, and moves ABSRAT macro from fact.cpp for reuse
src/CalcManager/Ratpack/support.cpp	Updates internal calls to use `_addrat`/`_subrat`/`_lograt` versions
src/CalcManager/Ratpack/trans.cpp	Updates internal calls to use `_subrat` version
src/CalcManager/Ratpack/transh.cpp	Updates internal calls to use `_subrat`/`_addrat` versions
src/CalcManager/Ratpack/itrans.cpp	Updates internal calls to use `_addrat`/`_subrat` versions
src/CalcManager/Ratpack/itransh.cpp	Updates internal calls to use `_addrat`/`_subrat`/`_lograt` versions
src/CalcManager/Ratpack/fact.cpp	Updates internal calls to use `_addrat`/`_subrat`/`_lograt` versions; removes local ABSRAT macro (now in ratpak.h)
src/CalcManager/Ratpack/logic.cpp	Updates internal call to use `_addrat` version
src/CalculatorUnitTests/CalculatorManagerTest.cpp	Adds test cases for sqrt(2.25) - 1.5 in both Standard and Scientific modes, and log(sqrt(2.25) / 1.5) test

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/CalcManager/Ratpack/exp.cpp

src/CalcManager/Ratpack/rat.cpp

Copilot · 2025-12-23T02:52:29Z

src/CalcManager/Ratpack/rat.cpp

+            DUPRAT(threshold, absA); 
+        }
+


This line has trailing whitespace that should be removed.

Suggested change

DUPRAT(threshold, absA);

}

DUPRAT(threshold, absA);

}

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

tian-lt · 2025-12-23T06:24:31Z

@guihuaz
Shall we adjust the result in the Rational class or a higher context? Because treating the epsilon as zero is more like an application requirement rather than a correct behavior of semi-numerical analysis.

guihuaz · 2025-12-23T07:29:57Z

@guihuaz Shall we adjust the result in the Rational class or a higher context? Because treating the epsilon as zero is more like an application requirement rather than a correct behavior of semi-numerical analysis.

@tian-lt I thought about adding the change in Rational for "+/-" and RationalMath for Log. I think within Ratpack is more appropriate.

_snaprat uses rat_smallest which is private to Ratpack implementation. Rational is a just wrapper, which doesn't know exactly when to snap.
More fundamentally, the snapping is not really just treating a small value as zero. Ratpack (and the Calculator app) should display the numbers within precision no matter how small it is. Take another example, 2e-47 - 1e-47, the Ratpack should return and Calculator app should display 1e-47, although the number is small. In the case of sqrt(2.25) - 1.5, the result -1.2566515751494548126925332972115e-47 is wrong answer from Ratpack, not just application requirement. Please see the PR description for more details.
Will Ratpack be used by other applications? If not, the risk of changing in Rational and RationalMath is similar as directly changing with Ratpack. By the way, Ratpack, Rational, and RationalMath handle both rational and irrational numbers. So their naming is not accurate.

tian-lt · 2025-12-24T02:00:49Z

The changes look good to me.
@HuanglinXu21 @guominrui, could you help take a look?

HuanglinXu21 · 2025-12-24T03:05:42Z

The changes look good to me. @HuanglinXu21 @guominrui, could you help take a look?

Looks good to me, too. I tested several cases, and they all passed😉

axtar · 2026-01-30T08:46:13Z

It seems that this PR has broken lograt. For example log of 9.e+99 now returns 0 instead of 99.954...

guihuaz · 2026-01-30T09:06:04Z

It seems that this PR has broken lograt. For example log of 9.e+99 now returns 0 instead of 99.54...

@axtar do you have dev env? If so, could you please try to try removing _snaprat inside lograt to see if it fixes the issue? I'm not on my windows machine, right now. I might have time to try this weekend.

axtar · 2026-01-30T09:30:30Z

@guihuaz removing _snaprat in lograt fixes the problem but then of course the problem with log( tan₀( 45 ) ) reappears.

axtar · 2026-01-31T10:34:29Z

@guihuaz As a temporary solution, I added a check in my project so that _snaprat only snaps to zero if absR is also smaller than rat_smallest.

guihuaz · 2026-01-31T19:02:49Z

src/CalcManager/Ratpack/rat.cpp

+    ABSRAT(absR);
+
+    // if absResult < threshold => snap to zero
+    if (rat_lt(absR, threshold, precision))


(@axtar , move comment here to have a thread)
Do you add a check here? I think this is safe change. Could you please send a PR to add the additional check absR < rat_smallest?

It may expose some other edge cases back, but _snaprat does not likely catch all these cases. In my understanding, the issue is fundamental design flaw in Ratpack. Ratpack, as the name implies, is for arbitrary precision rational math. But it tries to include some irrational math using approximation and rounding. I think this would inevitably introduce errors.

Yes, I have added an “&&” to that “if”.
I’m sorry but I don’t have the skills to contribute to this repository. I don’t even really know how to make a pull request. I’m simply using ratpak on a microcontroller for a hobbyist hardware calculator project. That’s why I found the issue.
Approximation and rounding issues are difficult to fix but for a calculator arbitrary precision is always better than 64-bit floating point arithmetic. I really like ratpak, it's small and very efficient. But it has a lot of memory leaks that I had to fix to run it on a microcontroller.

I have opened an issue for this

guihuaz added 2 commits December 7, 2025 00:57

add snap zero in add/sub/log

057ab31

add a comment

ac77ed4

guihuaz mentioned this pull request Dec 7, 2025

Wrong result for e.g. √(2.25) - 1.5 #222

Closed

tian-lt requested a review from Copilot December 23, 2025 02:48

Copilot started reviewing on behalf of tian-lt December 23, 2025 02:49 View session

Copilot AI reviewed Dec 23, 2025

View reviewed changes

guihuaz and others added 3 commits December 22, 2025 20:52

Update src/CalcManager/Ratpack/exp.cpp

f2e2d9d

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update src/CalcManager/Ratpack/rat.cpp

09f72fb

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update src/CalcManager/Ratpack/rat.cpp

4873cda

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

tian-lt approved these changes Dec 24, 2025

View reviewed changes

tian-lt merged commit ffd0519 into microsoft:main Dec 24, 2025
5 checks passed

guihuaz commented Jan 31, 2026

View reviewed changes

axtar mentioned this pull request Feb 6, 2026

PR #2424 leads to incorrect lograt results in some cases #2439

Open

Conversation

guihuaz commented Dec 7, 2025

Fixes #222, #309, #574

Description of the changes:

How changes were validated:

Uh oh!

guihuaz commented Dec 7, 2025

Uh oh!

guihuaz commented Dec 20, 2025

Uh oh!

tian-lt commented Dec 23, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Dec 23, 2025

Choose a reason for hiding this comment

Uh oh!

tian-lt commented Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guihuaz commented Dec 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tian-lt commented Dec 24, 2025

Uh oh!

HuanglinXu21 commented Dec 24, 2025

Uh oh!

Uh oh!

axtar commented Jan 30, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

guihuaz commented Jan 30, 2026

Uh oh!

axtar commented Jan 30, 2026

Uh oh!

axtar commented Jan 31, 2026

Uh oh!

guihuaz Jan 31, 2026

Choose a reason for hiding this comment

Uh oh!

axtar Jan 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

axtar Feb 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tian-lt commented Dec 23, 2025 •

edited

Loading

guihuaz commented Dec 23, 2025 •

edited

Loading

axtar commented Jan 30, 2026 •

edited

Loading

axtar Jan 31, 2026 •

edited

Loading