Add sqrt feature

[peter279k]

As title, it's related to issue #18.

[BenMorel]

NAN is a float, not a string; you cannot return NAN. We should throw an exception here. Actually we can even do nothing: Calculator is not part of the public API, and assumes that all input is already validated.

As such, we can perform the test only in the BigNumber classes, you can safely remove this check.

[BenMorel]

This looks like a good start! 👍

A few guidelines:

• you need to add an abstract sqrt() method to the base Calculator class, to be used by BigNumber classes
• the sqrt() method should be added to BigInteger: BigInteger sqrt() : BigInteger
• ideally, if you can handle scale, the sqrt() method should be added to BigDecimal: BigDecimal sqrt(int $scale) : BigDecimal (1)
• ideally, all rounding modes should be supported (2)
• We'll need a lot of tests, not targeted to invididual calculators, but to BigInteger and BigDecimal directly; some with values that have an exact square root, and others that must be rounded

(1) I don't mind if the sqrt() method only exists on BigInteger for now. We can always refactor later to add BigDecimal support with scale.

(2) I don't mind if, for now, a single rounding is performed (most likely, rounding down), but it needs to be documented, consistent across calculators, and well tested.

[peter279k]

@BenMorel, thank you for your reply. I think we can do the following works at this time:

• Adding an abstract sqrt() method to the base Calculator class, to be used by BigNumber classes
• The sqrt() method should be added to BigInteger: BigInteger sqrt() : BigInteger.

Can you explain more details about how to add the sqrt method to the BigInteger class?

Thanks.

[BenMorel]

Calculator is an internal class, it's meant to be consumed by BigInteger, BigDecimal and BigRational, but never be used directly by the end user.

So adding sqrt() only in Calculator makes it unavailable to users of the library, what they'll be looking for is BigInteger::sqrt() or BigDecimal::sqrt()!

Check how other methods are implemented (add(), mul() etc.) , they basically:

• validate the input
• perform the operation on the Calculator
• wrap the output in a BigInteger or BigDecimal class

[peter279k]

Hi @BenMorel, thank you for your reply. I've added the sqrt method inside BigInteger class.

Please check that. I also do the input validation, perform operation with Calculator class and return the BigInteger wrapper.

If having any problem, please write the comment on that line of code.

Thanks.

[BenMorel]

Hi @peter279k, I've actually fixed the issues above myself, and also added a good number of tests for BigInteger::sqrt().

It seems to work fine, however the current implementation of sqrt() in NativeCalculator is awfully slow: on my machine it takes roughly 1s to calculate the square root of a number with 100 digits.

This makes tests fail as they take too long to complete.

Of course NativeCalculator will always be slower than GMP or BCMath, but to give you an idea, the same sqrt() on GMP takes 0.000002 seconds (2 µs!), so there must be a better, faster algorithm to calculate the square root.

Can you please investigate? Thanks!

[peter279k]

Hi @BenMorel, thank you for your reply.

Actually I use the Newton method to calculate square root number.

I don't think other methods can be faster than this method. And other methods will be slower than Newton method.

[BenMorel]

Actually the issue with the tests is not the slowness of the algorithm, it's that there's an infinite loop. Try:

BigInteger::of('3')->sqrt();

Could you please fix this?

[peter279k]

@BenMorel, thank you for your reply. I will fix that later.

[BenMorel]

Why the last 2 commits? You explicitly created another infinite loop.

[peter279k]

@BenMorel, I misunderstand your request. I will revert them.

Forgive me I'm tired recently....

[peter279k]

Hi @BenMorel, after tracing source code, It looks strange.

Firstly, consider following approach:

$ans = 3;
$x = 3;
$del = 0;
$pre = 0;

while (abs($ans - $pre) > 0) {
    $pre = $ans;
    $ans = ($ans + $x / $ans) / 2;
}
echo $ans;

It worked fine when we let $ans=3 calculate sqrt result.

If we use this sqrt method, the result will happen infinite loop.

I guess the operator calculations have some problems and this will cause the infinite loop.

[peter279k]

Maybe we have to let the division operator calculation allow float string result?

I think it can solve this infinite loop problem.

[BenMorel]

I don't think the other operators are bugged, they're well tested, the problem most likely lies in the sqrt algorithm. And no, you can't allow float, this would defeat the whole purpose of an arbitrary precision library, that must deal with exact calculations of any size.

[peter279k]

Well, I think it's still about the numbers problem. Consider following code snippet:

$ans = 3;
$x = 3;
$del = 0;
$pre = 0;

while (abs($ans - $pre) > 0) {
    $pre = $ans;
    $ans = (int) ((int) ($ans +  (int) $x / $ans) / 2);
    var_dump($ans);
}
echo $ans;

It lets all division operator calculation result will be the integer. And this will cause the infinite loop problem.

[BenMorel]

You most likely adapted some code that is supposed to deal with floats, and trying to make it work with integer arithmetic. You need to fix this algo, or find another one.

Note that it works fine for other sqrt() tests, so it can probably work with a few tweaks, if you really take the time to understand it.

[BenMorel]

AFAICS in this Newton pseudocode, it looks like you're using a zero Epsilon (error tolerance), which returns the exact result but may end up with an infinite loop with some numbers in our case.

When I change cmp($checkedSqrtNumber, '0') to cmp($checkedSqrtNumber, '1'), the infinite loop is gone, but some tests fail, for example:

--- Expected
+++ Actual

-'7'
+'8'

-'66291728919896041699308382947726934'
+'66291728919896041699308382947726935'

Always wrong by 1 on the last digit. I guess this is where the issue lies, we need to use a zero epsilon indeed, but find a way to detect and prevent the infinite loop.

[BenMorel]

OK I changed the algorithm to the one described here:

https://cp-algorithms.com/num_methods/roots_newton.html

Which describes the problem well:

Another common variant of the problem is when we need to calculate the integer root (for the given n find the largest x such that x²≤n).

And now all tests pass. I'll merge this now, and will polish it later: we can speed up the algorithm by starting with a closer approximation (they use 2^(bits/2), but we can probably use something less expensive to calculate in NativeCalculator).

I'll probably release it later today. 👍

[BenMorel]

@peter279k This little change on the initial approximation made sqrt() nearly 20x faster!

There is always room for improvement :)

[peter279k]

@BenMorel, thank you for your reply. I appreciate your help and review.

I also find another way to calculate the square root number.

The latest Travis CI build log is available here and here is the code.

I will keep this finding the better way to optimize this sqrt calculation.

[BenMorel]

Released in 0.8.3 👍

[BenMorel]

@peter279k If you find a faster way, don't hesitate to open a pull request!

If you are interested, I would be happy if you could investigate:

• supporting sqrt() on BigDecimal
• supporting rounding modes

[BenMorel]

Note: added support for BigDecimal::sqrt() in 41eeaed.