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

integer_nthroot: Fixing OverflowError in integer_nthroot() #14707

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@sharmaadarsh563

sharmaadarsh563 commented May 10, 2018

integer_nthroot: Fixing OverflowError in integer_nthroot()

Fixes #14704

In function integer_nthroot(), line 76 breaks, raising the OverflowError when 'n' (the denominator) becomes a very large integer -- for eg., math.factorial(171). Therefore, this PR rounding the numerator to the integer value.

@asmeurer

This comment has been minimized.

Member

asmeurer commented May 10, 2018

Please add a test.

@jksuom

This comment has been minimized.

Member

jksuom commented May 11, 2018

It looks like the initial guess value obtained by rounding is too crude when y (and exp) is big. The two cases, big y and big n, should probably be separated for instance as follows:

except OverflowError:
    logy = _log(y, 2)
    exp = logy/n if logy >= n else 0
@@ -967,6 +967,7 @@ def test_powers():
def test_integer_nthroot_overflow():
assert integer_nthroot(10**(50*50), 50) == (10**50, True)
assert integer_nthroot(10**100000, 10000) == (10**10, True)
assert integer_nthroot(100, 10**1000) == (1, False)

This comment has been minimized.

@sidhantnagpal

sidhantnagpal May 12, 2018

Member
In [1]: from sympy.abc import a

In [2]: b = a**a

In [3]: c = b**b

In [4]: c.subs(a, -4)
Out[4]: 2**(31/32)/2

In [5]: c.subs(a, -5)
Out[5]: -(-1)**(3124/3125)*5**(1/625)

In [6]: c.subs(a, -6)
Out[6]: 6**(7775/7776)/6

In [7]: c.subs(a, -7)
Out[7]: -(-1)**(823542/823543)*7**(1/117649)

Looks good.
c.subs(a, -10) has high usage of system resources but overflow is no longer an issue.

This comment has been minimized.

@sharmaadarsh563

sharmaadarsh563 May 12, 2018

Yes, this PR correctly fixes the OverflowError issue. To verify, you can also try the following example too -- with or without this PR:

In [1]: from sympy.abc import a

In [2]: if __name__ == '__main__':
   ...:     b = a ** a
   ...:     c = b ** b
   ...:     c.subs({a: -144})

Can we approve and merge this PR? What is the procedure?

@sharmaadarsh563

This comment has been minimized.

sharmaadarsh563 commented May 12, 2018

@asmeurer any suggestions?

@jksuom

This comment has been minimized.

Member

jksuom commented May 17, 2018

c.subs(a, -10) has high usage of system resources but overflow is no longer an issue

This is probably partly caused by the computation of t = x**n in the first loop at the end:

while t < y:
    x += 1
    t = x**n

In this loop, x (incremented by 1) is at least 2 so x**n has bit length at least n. If n is very big (say 1000000), then an integer t of that size is created in memory and then immediately abandoned (in most cases) after seeing that it is much bigger than y. The only possibility of y being bigger than t is that its bit length is bigger than n. Otherwise the loop could be skipped.

if y.bit_length() > n:
    while t < y:
        x += 1
        t = x**n

With a patch of this type it should be possible to add a test for integer_nthroot(y, n) where y = n = 144**144.

@sidhantnagpal

This comment has been minimized.

Member

sidhantnagpal commented May 17, 2018

Can we speed up inequality tests (after the guess has been made) by floating point comparison of their log values (assuming they won't be too big)?
x**n < y to n * log(x) < log(y) (or maybe similar expression like n < log(y)/log(x) whichever minimizes the possibility of error), that way, only log(x) will need to be calculated per iteration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment