Fix for #1799

[ds26gte]

• Extra arg removed for BigInteger.prototype.{log,round,roundEven,tan,atan,cos,sin,acos,asin}
• bnpExp() should take errbacks arg and use it if exponent too large
• bnModPowInt() should take errbacks arg and pass it along to bnpExp()
• bnPow() should take errbacks arg and pass it along to bnpExp()

[blerner]

@ds26gte , the fact that the tests above have passed, without this PR including any new tests at all, indicates a certain lack of testing coverage of the functionality being changed, yes? At minimum, please add regression tests for every function whose behavior now really ought to trigger a proper Pyret error, and make sure that it actually does so.

[ds26gte]

I've now improved the definition of num-log so that

num-log(num-expt(10, 36789)) -> ~84709.80298615796

Indeed

num-log(num-expt(36789, 36789)) -> ~386761.0708287983

(We can tighten these values considerably (using Racket as a comparison point), by changing the firstFewLen value in the code. I arbitrarily chose 10 for it. We should be able to hike it all the way to 308 or so.)

Previously these caused roughnum overflows (or before my recent changes, failed to invoke the throwDomainError method correctly because of the error in how errbacks was passed).

Unfortunately or (I think) fortunately, with the latest fix to num-log, it is now impossible to test that it produces roughnum overflow for sufficiently large input. Any number expression that doesn't already overflow necessarily has a logarithm that is (much) smaller than itself, so it will a fortiori not overflow.

I've added a test that shows num-expt raises a too large error for large inputs (and not converge to 1, as previously).

I've also added a test that shows num-log is now more capable.

Note that js-numbers already had explicit conditional tests bracketing the domain for asin(), acos(), so it serendipitously didn't matter that the original definitions for method versions of these functions had scribal errors.

[ds26gte]

@ds26gte , the fact that the tests above have passed, without this PR including any new tests at all, indicates a certain lack of testing coverage of the functionality being changed, yes? At minimum, please add regression tests for every function whose behavior now really ought to trigger a proper Pyret error, and make sure that it actually does so.

I've done so. Note that because of the way the non-method function and its method counterparts are written (e.g., acos vs BigInteger.prototype.acos), it turns out that the incorrect parameter signature on the method wasn't really creating a problem for the most part. Both the errbacks parameter and the n parameter were being ignored (because the methods use this directly). It was a problem for log, which wasn't showing the proper error, because it was treating a Pyret number object as errbacks. My latest fix to log however makes it converge for all args, so there is never any error anymore. I have added tests that showcase that log indeed works for large numbers where previously it didn't.

expt was plain wrong: it was converging to 1 when it shouldn't. Fixed that too.

This PR should go in sooner rather than later because it addresses numerical misbehavior that is immediately visible to the user.

[jpolitz]

I'm trying to understand the meaning of the “firstFew” here.

I think the idea is that in a number like

yyyyyyyyyyXXXXXXXXXXXXXXXXXX

Taking the log, which produces a Roughnum, is going to give the same answer regardless of the values of the XX... part. Is that right?

How much have we tested the boundary there/how can I be confident about or understand that boundary better?

[ds26gte]

I'm trying to understand the meaning of the “firstFew” here.

I think the idea is that in a number like

yyyyyyyyyyXXXXXXXXXXXXXXXXXX

Taking the log, which produces a Roughnum, is going to give the same answer regardless of the values of the XX... part. Is that right?

How much have we tested the boundary there/how can I be confident about or understand that boundary better?

At that point in the code, the incoming number is very large. If it had been had convertible to a fixnum, we would have already taken the JS log of the fixnum. But now, since it's so large, we can ignore its fractional part and view it as a big integer yyyyyyyyyyXXXXXXXXXXXXXXXXXX.

We can rewrite it as yyyyyyyyyy * 10^nX, deliberately losing some insignificant digits (nX == the number of X's). The base-10 log of this is simply log₁₀(yyyyyyyyyy) + nX, and is easily accomplished in JavaScript. It is now simple arithmetic to get the base-e log from this.

The "string" length of yyyyyyyyyy is our choice, and that is the firstFewLen in my code. I've chosen it to be 10, and for num-log(num-expt(10, 36789)), it returns a value ~84709.80298615796. The Racket result for the same expression is 84709.80298615794, so it differs only at the final, 11th place after the decimal point. To get it to exactly match Racket, firstFewLen has to empirically be 13, maybe 14.

Our choice of firstFewLen should obviously not be longer than the integral part of the incoming number. Note that the incoming number is very large, because it could not be converted to a JS fixnum. Thus it must be larger than 10^308, which is close to the largest JS fixnum. So we can have firstFewLen be all the way up to 308, but that's definitely overkill, and doesn't provide much more accuracy than firstFewLen == 10 or 14.