0

I'm trying to solve a projecteuler puzzle detailed below. My current function works for the numbers 1 to 10, but when I try 1 to 20 it just loops forever without a result.

2520 is the smallest number that can be divided by each of the numbers from 1 to 10 without any remainder. What is the smallest positive number that is evenly divisible by all of the numbers from 1 to 20?

def calculate():
    results = dict()
    target = 20
    num_to_test = 1
    while len(results) < target:
        for j in range(1, target+1):
            results[num_to_test] = True
            if num_to_test % j != 0:
                # current num_to_test failed in the 1-10, move on
                del results[num_to_test]
                break

        num_to_test += 1
    return min(results)

Can anyone see any issues in the logic, and especially I'd like to know why it is working for a target of 10, but not 20. Thanks

reaperscleapers
  • 1
  • 1
  • 1
  • 1
    I think this should help you: https://www.google.com/search?q=efficient+lcm+algorithm – Sundar R Jul 31 '13 at 20:23
  • 2
    [this is relevant](http://stackoverflow.com/questions/185781/finding-the-lcm-of-a-range-of-numbers) – Stephan Jul 31 '13 at 20:24
  • Check out this link to another stack overflow article. http://stackoverflow.com/questions/8024911/project-euler-5-in-python-how-can-i-optimize-my-solution – trueamerican420 Jul 31 '13 at 20:25
  • 1
    For 1-10 you shouldn't have to worry about 1, 2, 3, 4, or 5 since 6, 7, 8, 9, and 10 are multiples of them. This should significantly reduce the work the program will have to do. Since all numbers 11-20 are multiples of 1-10, this question can become `What is the smallest positive number that is evenly divisible by all of the numbers from 11 to 20?` – scohe001 Jul 31 '13 at 20:27
  • I could suggest you an efficient alogrithm without providing the code, but not sure if it's appropriate – Roman Pekar Jul 31 '13 at 20:53

5 Answers5

5

Your algorithm is pretty inefficient, but the core of your problem is that your results dictionary is accumulating 1 value for each integer that's evenly divisible by the numbers from 1-20, and your while loop is trying to keep going until it has 20 such numbers.

This is one correct way to implement this inefficient algorithm:

def calculate():
    target = 20
    candidate = 1
    success = False
    divisors = range(1, target+1)
    while not success:
        for divisor in divisors:
            if candidate % divisor != 0:
                candidate += 1
                break
        else:
            success = True

    return candidate

Note that the else clause really is on the for loop, not the if. From the tutorial on flow control:

Loop statements may have an else clause; it is executed when the loop terminates through exhaustion of the list (with for) or when the condition becomes false (with while), but not when the loop is terminated by a break statement.

A somewhat more concise expression would be:

candidate = 0
while not success:
    candidate += 1
    success = all((candidate % divisor == 0 for divisor in divisors))

That uses a generator expression so all can short-circuit and avoid doing unnecessary calculation.

Since this is a puzzle I'll pass on suggesting better algorithms.

Peter DeGlopper
  • 36,326
  • 7
  • 90
  • 83
4

actually I have very efficient algorithm for that problem. I'll not give you the code, but I could show you the way

For N = 10

1.Calculate all factors of all numbers from 5 to 10:

 [[2, 3], [7], [2, 2, 2], [3, 3], [2, 5]]

2.calculate maximum number of each prime in the list

 {2: 3, 3: 2, 5: 1, 7: 1}

3.get product of key power value

 2^3 * 3^2 * 5 * 7 = 2520
Roman Pekar
  • 107,110
  • 28
  • 195
  • 197
2

A lot of the other answers mention the original code being inefficient, but they still loop through almost every number. Wouldn't it be more efficient to utilize an lcm function?

def calculate(num, current_lcm = 1):
    if (num == 1): return current_lcm
    return calculate(num - 1, lcm(num, current_lcm))

def lcm(a, b):
    return a * b // gcd(a, b)

def gcd(a, b):
    while b:      
        a, b = b, a % b
    return a

print calculate(20)
scohe001
  • 15,110
  • 2
  • 31
  • 51
  • `from fractions import gcd` – John La Rooy Jul 31 '13 at 20:47
  • Out of 10 runs I get an average of `0:00:00.018344` with the gcd function I have up there and `0:00:00.018619` with fractions' function. This was not counting the time it took to import the function itself from fractions – scohe001 Jul 31 '13 at 20:50
  • Without prints and with 50 runs this becomes `0:00:00.000013` vs `0:00:00.000015` with a target of 10. As the target gets bigger, the code I used gets faster and faster in comparison to Fractions. With a target of 150 averaging 50 runs, the code above runs at `0:00:00.000338` vs `0:00:00.000357` – scohe001 Jul 31 '13 at 20:56
  • The library `gcd` is exactly the same code. Even the variables have the same names. – John La Rooy Jul 31 '13 at 21:02
  • @gnibbler then why is it running slower? – scohe001 Jul 31 '13 at 21:05
  • I don't know. Are you using `from fractions import gcd`? – John La Rooy Jul 31 '13 at 21:51
  • @gnibbler I'm on a different machine now, so I can't test again with that, but I was doing `import fractions` and then `fractions.gcd(a, b)` – scohe001 Aug 01 '13 at 00:45
  • @gnibbler Using `from fractions import gcd` drops the run time but about 2%, but using the local function still remains faster. I'm not sure why this is, but if you or anyone knows, I'd love to figure out why this is happening – scohe001 Aug 01 '13 at 20:18
1

Dont store em all, instead just return early when you find it, get rid of that result dictionary, this is not optimal at all by the way, just a clean up

def calculate():
    target = 20
    num_to_test = 0
    while True:
        num_to_test += target
        if all((num_to_test % j == 0) for j in range(1,target+1)):
            return num_to_test
    return -1

Also you dont need to test numbers that aren't multiples of your maximum. It'll run 20 times faster.

I switched to using a generator to test to see if the number was divisible by all() of the nubmers from 1 to 20

Props for writing your own algorithm and not copying one :)

Stephan
  • 16,509
  • 7
  • 35
  • 61
  • Don't forget to add the definition of the dictionary in there. Also, when I run your code with 10 as the target it gives me 20. 20 as the target produces 40. Thoughts? – scohe001 Jul 31 '13 at 20:43
  • i explicitly took out the dictionary, hang on its wrong @josh – Stephan Jul 31 '13 at 20:44
  • If you explicitly took out the dictionary, why are you still referencing it in your script? – scohe001 Jul 31 '13 at 20:57
1

While your algorithm is very inefficient, it may help a little to make this small change

        if num_to_test % j = 0:
            results[num_to_test] = True
        else:
            # current num_to_test failed in the 1-10, move on
            break

Not sure why you are storing them all though? For debugging perhaps?

Hint. It would be better to calculate the prime factors of the result and simply multiply those together.

# spoiler  






























def calculate(target):
    n = 1
    for i in range(1, target+1):
        for j in range(1, target+1):
            if (n * j) % i == 0:
                n *= j
                break
    return n
John La Rooy
  • 295,403
  • 53
  • 369
  • 502