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

Fixes the Sqrt accuracy (#97) #98

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lipchev
Copy link
Contributor

@lipchev lipchev commented Dec 22, 2024

Fixes #97

@@ -47,7 +48,7 @@ public void The_result_should_be_equal_to_Math_Sqrt(double value) {
var expected = Math.Sqrt(value);
var fraction = Fraction.FromDouble(value);
// Act
var actual = fraction.Sqrt();
var actual = fraction.Sqrt(15);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note: this test broke as it started actually using the default 30 digits accuracy..

@lipchev
Copy link
Contributor Author

lipchev commented Dec 22, 2024

As expected- the performance is now way worse:

Old Method FractionToRaise Mean Error StdDev Gen0 Allocated
Sqrt 2/3 1,282.094 ns 48.5405 ns 2.6607 ns 0.0286 504 B
Sqrt 2 1,236.765 ns 54.4513 ns 2.9847 ns 0.0210 376 B
Sqrt 3 1,311.985 ns 121.7756 ns 6.6749 ns 0.0286 504 B
Sqrt 1024 307.165 ns 30.2575 ns 1.6585 ns 0.0081 136 B
New Method FractionToRaise Mean Error StdDev Gen0 Allocated
Sqrt 2/3 4,088.5 ns 360.59 ns 19.77 ns 0.0763 1320 B
Sqrt 2 3,851.2 ns 176.99 ns 9.70 ns 0.0610 1072 B
Sqrt 3 4,119.6 ns 82.33 ns 4.51 ns 0.0687 1200 B
Sqrt 1024 327.5 ns 69.94 ns 3.83 ns 0.0081 136 B

but I think there are a couple of things that could make things a little better.. Let me setup a new benchmark..

@lipchev
Copy link
Contributor Author

lipchev commented Dec 26, 2024

TLDR: One way or another this fix would be a breaking change- you have to decide in what way.

Right, so this is quite interesting: you know the accuracy parameter, having a default value of 30 digits after the decimal point. As we've established, the actual number of digits that was returned didn't necessarily match the specified accuracy (in terms of decimal places). By doing just a single iteration of the Babylonian method, the resulting number would have actually had around 30 (more like 32) significant digits of accuracy: 15-16 significant digits from the Math.Sqrt, which are doubled by doing a single iteration.

So? What do we care about the significant digits? The xml-docs says "number of digits after the decimal point" and the comparison of the absolute difference with 1 / 10 ^ accuracy is clearly meant for this purpose. Well, this isn't obvious from the numbers we used in the previous benchmarks, but lets consider two examples:

  • Sqrt(1.234e-100) : in this extreme scenario, the resulting square root (1.11E-50) is actually smaller than the specified accuracy- meaning that we don't actually need to do any iterations, we could simply return the result of Math.Sqrt and be sure that we've got the first 65 decimals right (even though, we've only got 15-16 significant digits). In fact, we don't even need the Math.Sqrt at this point- we could simply return Zero and that would still be "accurate enough" for the given accuracy of 30.
    In the less extreme cases (of the input being <1), we don't really need more than one iteration before reaching an accuracy of 30. For an accuracy of 100 we'd need another two iterations (with the accuracy scaling as 15, 30, 60, 120).
  • Sqrt(1.234e100): this is another extreme example, but the same effects can be observed with much less extreme values (>1)- it's just easier to demonstrate the problem, using scientific notation.
    The result of Math.Sqrt(1.234e100) is "1.11e+50". At this point we've achieve an accuracy of "-35" (scale of e+50 with just 15 significant digits). With the first newGuess we double the 15 significant digits, reaching an accuracy of "-17". So, we've still got another 17 junk digits leading the decimal separator and the terms of our newGuess are already quite large A = 1.23E+100, B = 1.11E+50. The absoluteDifference (which should become smaller than 1 / 10 ^ accuracy) is still at "2.49e+33"... Still a long way to go: let's do another iteration - having resolved the first 33 digits, the next iteration would double the precision once again- reaching a total of 66 correct digits. For our value of "1.11e+50" that's still just 16 decimal places. The absoluteDifference remains on the large side: "2.79e+16", so we keep going. Normally the next iteration would have once again doubled in size, giving us another 66 correct digits (for an accuracy of 82), but as a cost saving measure, I've been discarding the non-significant digits from every iteration up until now, so our new accuracy is only "50", but hopefully you see how the progression goes... (it's actually even worse if we had to rely on the absoluteDifference- as it still wouldn't have reached it's intended target).

Overall, no matter the numbers- the convergence is very fast (in terms of iterations), but there is a clear asymmetry with respect to the input scale. A typical number such as 1.234 would only take 1 iteration in order to reach an accuracy of 30, but the number 1234 would likely take 2, resulting in an accuracy that's between 56-58 decimal places (almost twice what we asked for).
On the other hand, if we decide to turn the "accuracy" into "significant digits"- then we have to decide what we to do with the "insignificant" digits: not sure if there is much sense in returning the number 1.11e+50 with a non-zero denominator, while there are 20 junk digits preceding the radix (and there is no build-in Fraction.Round overload that accepts a number of significant digits).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Square root accuracy is ignored
1 participant