0

I'm trying to find the largest prime factor for a number. The code runs correctly on IDLE when used with smaller numbers, but doesn't seem to print anything to the screen at all when I assign a larger number like 600851475143 to n. Why?

def isPrime(n):
    isPrime = True
    for i in range(2,n-1):
        if n % i == 0:
            isPrime =  False
        return isPrime

largest = 0
n = 600851475143
for i in range(2,n-1):
    if isPrime(i) and n % i == 0:
        largest = i
        n = n / i
        continue

print("The largest prime factor is", largest)

I'm running Python 3.3, by the way.

==============================================================================

Thanks everyone!

I fixed my original code as follows:

def isPrime(n):
    for i in range(2,n-1):
        if n % i == 0:
            return False
    return True

largest = 0
n = 600851475143
for i in range(2,n-1):
    if isPrime(i) and n % i == 0:
        largest = i
        if i == n:
            break
        n = n / i

print("The largest prime factor is", largest)

Like nakedfanatic said, their code runs faster, and I edited it slightly:

largest = 0
n = 600851475143
i = 2
while True:
    if n % i == 0:
        largest = i
        if n == i:
            # finished
            break
        n = n / i
    else:
        i += 1

print("The largest prime factor is", largest)
bard
  • 2,440
  • 5
  • 29
  • 44
  • 3
    You only need to check as far as sqrt(n). Any factors larger than that will have a corresponding factor smaller than that. You can find the larger one by simple division –  Sep 25 '13 at 04:00
  • 3
    Is it not printing or *not finished* calculating? – justhalf Sep 25 '13 at 04:01

7 Answers7

2

There are several areas of optimization:

  • all factorization only needs to got up to sqrt(n) (inclusive)

  • convert isPrime() to a table lookup

    Initialize a lookup table using n, then you compute all primes < sqrt(n) only once and loop through them.
    As comments pointed out, this takes up large memory space. We can use bit flag to cut the memory requirement to 1/8, and we can cut it by a further half if we skip all the even numbers (then have to test if n is even separately). But that may still be daunting for LARGE n.

  • (if using current code) return early in isPrime() (by @justhalf)

  • loop backwards (from sqrt(n) to 2) when looking up the largest factor

  • return early if the quotient is 1 after dividing by a factor (by @justhalf)

  • This post (suggested by @prashant) contains more complicated algorithm (making my suggestion very naive ><):

    Fastest way to list all primes below N

  • ... (edits are welcome)

Community
  • 1
  • 1
leesei
  • 5,812
  • 2
  • 24
  • 46
  • 2
    Regarding point number 2 ... a table lookup is good if you're only dealing with small primes -- But if you want to store a table of all the primes up to a really big number ... It might take a lot of memory :). Also, in your looping, you can always skip even numbers :) – mgilson Sep 25 '13 at 04:07
  • In an upwards loop you can actually skip any number that is a multiple of any factor that you've already checked for before. – Shashank Sep 25 '13 at 04:08
  • @ShashankGupta If I'm not misunderstood, the OP only need the largest prime factor, not the list of all prime factors. – leesei Sep 25 '13 at 04:11
  • These are just optimizations for an inherently wrong algorithm. You don't need a prime lookup table, and you don't need an isPrime function at all. – nakedfanatic Sep 25 '13 at 04:41
  • @nakedfanatic I agree xD There is a much smarter way to do this. But I was only listing optimizations for the algorithm the OP was using which is very similar to (1. list all primes of N from smallest to greatest 2. return last prime found)More information on the correct algorithm can be found here: http://math.stackexchange.com/questions/389675/largest-prime-factor-of-600851475143 – Shashank Sep 25 '13 at 04:56
1

Most likely, your code isn't terminating with large n, simply because it takes so long to run through the loop.

qaphla
  • 4,527
  • 3
  • 17
  • 29
1

It's because you keep trying even if n is already 1.

This code will help you to see the problem:

def isPrime(n):
    for i in xrange(2,n-1):
        if n % i == 0:
            return False
    return True

largest = 0
n = 600851475143
for i in xrange(2,n-1):
    print 'Checking whether %d divides %d' % (i,n)
    if isPrime(i) and n % i == 0:
        largest = i
        n = n / i
        continue

print("The largest prime factor is", largest)

which will produce:

...
Checking whether 6857 divides 6857
Checking whether 6858 divides 1
Checking whether 6859 divides 1
Checking whether 6860 divides 1
Checking whether 6861 divides 1
Checking whether 6862 divides 1
Checking whether 6863 divides 1
Checking whether 6864 divides 1
Checking whether 6865 divides 1
Checking whether 6866 divides 1
Checking whether 6867 divides 1
Checking whether 6868 divides 1
Checking whether 6869 divides 1
Checking whether 6870 divides 1
Checking whether 6871 divides 1
Checking whether 6872 divides 1
Checking whether 6873 divides 1
...

You should break the loop when n becomes 1, so that it won't do unnecessary checking

n = n / i
if n==1:
    break
continue

And anyway, your code might be improved by a lot, haha, see others' suggestions.

justhalf
  • 8,635
  • 2
  • 43
  • 69
1

Your code is running in O(n²) time, which means it will quickly become unreasonably slow as the size of n increases. That is why your algorithm works for small values, but hangs for large values.

This code does the same thing in O(n) time without doing any prime checking at all, and returns a result instantly:

prime_factors = []
n = 600851475143
i = 2
while True:
    if n % i == 0:
        prime_factors.append(i)
        if n == i:
            # finished
            break
        n = n / i
    else:
        i += 1

print("The largest prime factor is", prime_factors[-1])
nakedfanatic
  • 2,728
  • 2
  • 26
  • 31
1

More difficult problems may require a different algorithm.

Check this question out: Fastest way to list all primes below N

Your code looks okay, but could take a long time for a large n. Leveraging math can enable you to do this problem orders of magnitude faster.

On that link, I recommend rwh_primes1 for a pure python solution, and primesfrom3to as one that uses numpy. Both of those implementations are fairly short, relatively clear, and do basically the same thing. Those code snippets are written in Python 2, so a translation might look like this:

def rwh_primes1(n):
    sieve = [True] * (n//2)
    for i in range(3, int(n**0.5)+1,2):
        if sieve[i//2]:
            sieve[i*i//2::i] = [False] * ((n-i*i-1)//(2*i)+1)
    return [2] + [2*i+1 for i in range(1,n//2) if sieve[i]]
Community
  • 1
  • 1
Prashant Kumar
  • 14,945
  • 14
  • 46
  • 63
0
isPrime = True
for i in range(2,n-1):
    if n % i == 0:
        isPrime =  False
    return isPrime

This loop always exits the first iteration due to the unconditional return. Try:

for i in range(2,n-1):
    if n % i == 0:
        return False

return True

Also the upper bound n-1 can be reduced to sqrt(n)+1.

John Kugelman
  • 307,513
  • 65
  • 473
  • 519
0

Another aspect of your code which may be slowing it down is the second half of your code

largest = 0
n = 600851475143
for i in range(2,n-1):
    if isPrime(i) and n % i == 0:
        largest = i
        n = n / i
        continue

Specifically the statement

if isPrime(i) and n % i == 0: 

Per the documentation, the second condition is only evaluated if the first one is True. In your case it would make more sense to reverse the conditions so that computationally les expensive division is performed always and the more expensive isPrime() is only called for the actual factors

largest = 0
n = 600851475143
for i in range(2,n-1):
    if n % i == 0 and isPrime(i):
        largest = i
        n = n / i
        if n == 1: 
            break
RedBaron
  • 4,529
  • 5
  • 35
  • 60