Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

A faster totient function #1086

Closed
wants to merge 7 commits into from

3 participants

@catchmrbharath

The old totient function was iterating over all numbers less than n and calculating whether the gcd is 1 which would take O{n*logn} time.

The new totient function iterates over the primes less than sqrt(n) (which is cached I think) and calculates the totient function. This will take O{sqrt(n)} time.

sympy/ntheory/residue_ntheory.py
((5 lines not shown))
n = int_tested(n)
if n < 1:
raise ValueError("n must be a positive integer")
- tot = 0
- for x in xrange(1, n):
- if igcd(x, n) == 1:
- tot += 1
+ tot = n
+ for x in sieve.primerange(1, int(sqrt(n))):
@smichr Collaborator
smichr added a note

it's kind to use p (for prime) for the loop variable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
sympy/ntheory/residue_ntheory.py
((5 lines not shown))
n = int_tested(n)
if n < 1:
raise ValueError("n must be a positive integer")
- tot = 0
- for x in xrange(1, n):
- if igcd(x, n) == 1:
- tot += 1
+ tot = n
+ for x in sieve.primerange(1, int(sqrt(n))):
+ if n % x == 0:
@smichr Collaborator
smichr added a note

Could you import multiplicity from factor_ and do this:

m = multiplicity(p, n):
if m:
factor = p**m
tot -= tot//factor
n //= factor

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

@smichr I have used the multiplicity function in the new commit.

@smichr

dedent 4 spaces.

@smichr

a space goes after the comma

@catchmrbharath

Sorry. My bad. Fixed it.

@hector1618

@catchmrbharath Are you sure about the logic(or the catch)?
First, primerange(1, int(sqrt(n)) will exclude the last prime which you should consider. For example,

In [1]: sieve.primerange(1,7)
Out[1]: <generator object primerange at 0x9fbce64>

In [2]: for x in _ :
   ...:     print x
   ...:     
   ...:     
2
3
5

Second, what about the prime which are larger than sqrt(n) but divides n? Ex. n = 20 and from your logic, it only consider prime 2, 3 but not 5 which is a factor of 20 and thats why the following difference.

In [11]: totient_(20) # Your modified function
Out[11]: 12

In [12]: t = 0;

In [13]: for i in range(1, 20):
   ....:     if igcd(i, 20) == 1:
   ....:         t = t + 1
   ....:         
   ....:         

In [14]: t == totient_(20)
Out[14]: False

In [15]: t
Out[15]: 8
@hector1618

@smichr Why do we have two separate function viz totient and totient_ ? And those two are at different locations sympy/ntheory/factor_.py and sympy/ntheory/residue_ntheory.py respectively.

@catchmrbharath

@hector1618
About the sqrt(n) problem : You were right. I have fixed it.

About the second problem : The idea is that there can be maximum of one prime factor greater than sqrt(n) . That is taken care by the if n > 1 : tot -= tot // n. The proof goes like this: if p1 and p2 are prime factors greater than sqrt(n) then p1*p2 will be greater than n, and hence its not possible.

I introduced an error when I changed the function to include multiplicity. I should have done `tot -= tot // p rather than tot -= tot //factor. It still passed the tests though.

I have fixed both the issues. Also as hector1618 said, there are two totient functions.

@hector1618

@catchmrbharath : I got the logic. Good work.
I think its good to go in.

@smichr
Collaborator
@smichr
Collaborator
@smichr
Collaborator

The test that identified the logical error should probably be added, too. Also, dumb is sometimes faster than smart, so if a change is going to be made we should make sure that our assumptions about the smarter being faster are correct. In this case they aren't:

(tot is your current function)

>>> t=time();jnk=[tot(i) for i in xrange(1,1000)];print time()-t
1.98099994659
>>> t=time();jnk=[tot(i) for i in xrange(1,1000)];print time()-t
1.02399992943
>>> t=time();jnk=[totient(i) for i in xrange(1,1000)];print time()-t
0.0240001678467
>>> t=time();jnk=[totient_(i) for i in xrange(1,1000)];print time()-t
2.27099990845
@smichr
Collaborator

Maybe your first approach (without multiplicity) was better. But compare it and post the result here.

@smichr
Collaborator

I think I had the two functions (totient_ and totient0 reversed. You were replacing totient_; it's simple and slow. Your modifications are an improvement, but not better than the existing totient (unless you can come up with something better). So I think we should just delete totient_, update imports and documentation, and thank you and @hector1618 for helping us get rid of the inefficient algorithm.

@catchmrbharath

The other totient function uses the same algorithm which I use. So I think that is the best solution.

There were no tests for totient_ . The method was only used in residue_ntheory file. So fixing it was easy. I have removed the totient_ function from init.py also. What should I do to update the documentation?

@smichr
Collaborator
@catchmrbharath

yeah done.

sympy/ntheory/residue_ntheory.py
@@ -56,10 +34,11 @@ def n_order(a, n):
>>> n_order(4, 7)
3
"""
+ from sympy.ntheory import totient
@smichr Collaborator
smichr added a note

Here and below, import at the top (from factor_ import factorint, trailing, totient). It's only a last restort that we import in the function.

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

I rebased and squashed and added int_tested to totient and committed from here. So thanks: this is in.

@smichr smichr closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
View
2  doc/src/modules/ntheory.txt
@@ -89,8 +89,6 @@ Ntheory Functions Reference
.. autofunction:: int_tested
-.. autofunction:: totient_
-
.. autofunction:: n_order
.. autofunction:: is_primitive_root
View
2  sympy/ntheory/__init__.py
@@ -9,6 +9,6 @@
pollard_pm1, pollard_rho, primefactors, totient, trailing, divisor_count
from partitions_ import npartitions
from residue_ntheory import is_primitive_root, is_quad_residue, \
- legendre_symbol, jacobi_symbol, n_order, totient_
+ legendre_symbol, jacobi_symbol, n_order
from multinomial import binomial_coefficients, binomial_coefficients_list,\
multinomial_coefficients
View
28 sympy/ntheory/residue_ntheory.py
@@ -1,6 +1,6 @@
from sympy.core.numbers import igcd
from primetest import isprime
-from factor_ import factorint, trailing
+from factor_ import factorint, trailing, totient
def int_tested(*j):
"""Return all args as integers after confirming that they are integers.
@@ -19,28 +19,6 @@ def int_tested(*j):
return i[0]
return i
-def totient_(n):
- """Returns the number of integers less than n and relatively prime to n.
-
- Examples
- ========
-
- >>> from sympy.ntheory import totient_
- >>> totient_(6)
- 2
- >>> totient_(67)
- 66
-
- """
- n = int_tested(n)
- if n < 1:
- raise ValueError("n must be a positive integer")
- tot = 0
- for x in xrange(1, n):
- if igcd(x, n) == 1:
- tot += 1
- return tot
-
def n_order(a, n):
"""Returns the order of a modulo n
@@ -59,7 +37,7 @@ def n_order(a, n):
a, n = int_tested(a, n)
if igcd(a, n) != 1:
raise ValueError("The two numbers should be relatively prime")
- group_order = totient_(n)
+ group_order = totient(n)
factors = factorint(group_order)
order = 1
if a > n:
@@ -102,7 +80,7 @@ def is_primitive_root(a, p):
raise ValueError("The two numbers should be relatively prime")
if a > p:
a = a % p
- if n_order(a, p) == totient_(p):
+ if n_order(a, p) == totient(p):
return True
else:
return False
Something went wrong with that request. Please try again.