calculating the sum of divisors code won't work properly Python

Question

I have a code that should calculate the sum of divisors of a given number but it just won't work the way it should, what am I missing????

here is the code:

def sum_divisors(n):
  sumlist = []
  for i in range(n ,0 , -1):
    if n % i == 0:
      sumlist.append(i)
      n -= 1
    elif n % i != 0:
      pass
  fsum = sum(sumlist)
  return fsum

print(sum_divisors(0)) # sum of 0 = 0
print(sum_divisors(3)) # Should sum of 1 = 1
print(sum_divisors(36)) # Should sum of 1+2+3+4+6+9+12+18 = 55
print(sum_divisors(102)) # Should be sum of 2+3+6+17+34+51 = 144

and this is what I get as results: 0 6 666 5253

I'm still a beginner in this so any help will be most appreciated.

Answer 1

Two problems:

• n -= 1 is wrong and ultimately leads to i and n always being equal, therefore n % i == 0 always being true (therefore the large numbers you get) => delete this line
• i shouldn't start from n, since based on your expected results you don't want the number itself to be included in the sum of divisors. Therefore: range(n - 1, 0, -1) insteaf of range(n ,0 , -1)

Apart from this, your code has some unnecessary parts, like the elif. Consider GGberry's version for a cleaner style.

---

Or use a generator expression instead of the loops:

def sum_divisors(n):
    return sum(i for i in range(1, n) if not n % i)

Answer 2

This should work.

def sum_divisors(n):
    sum_divs = []

    for i in range(1, n):
       if n % i == 0:
           sum_divs.append(i)

    return sum(sum_divs)

print(sum_divisors(0)) # sum of 0 = 0
print(sum_divisors(3)) # Should sum of 1 = 1
print(sum_divisors(36)) # Should sum of 1+2+3+4+6+9+12+18 = 55
print(sum_divisors(102)) # Should be sum of 1+2+3+6+17+34+51 = 114

Answer 3

Lesson in debugging of your approach - there are better ones:

def sum_divisors(n):
    sumlist = []
    for i in range(n ,0 , -1):
        if n % i == 0:
            sumlist.append(i)
#      n -= 1           # this does nothing valuable
#    elif n % i != 0:   # this does nothing
#      pass             # this does nothing
    fsum = sum(sumlist)

    # debugging what your func computes by printing it:
    print (n, sumlist, fsum, sep = " ==> ")  

    return fsum

sum_divisors(0) # sum of 0 = 0
sum_divisors(3) # Should sum of 1 = 1
sum_divisors(36) # Should sum of 1+2+3+4+6+9+12+18 = 55
sum_divisors(102) # Should be sum of 2+3+6+17+34+51 = 144 (wrong sum)

Outputs:

0 ==> [] ==> 0
3 ==> [3, 1] ==> 4
36 ==> [36, 18, 12, 9, 6, 4, 3, 2, 1] ==> 91
102 ==> [102, 51, 34, 17, 6, 3, 2, 1] ==> 216

According to what you want as result - you start to test your range with n-1 instead of n to get there:

def fixed_sum_divisors(n):
    sumlist = []
    for i in range(n-1 ,0 , -1): # dont start with n
        if n % i == 0:
            sumlist.append(i)
    fsum = sum(sumlist)
    print (n, sumlist, fsum, sep = " ==> ")
    return fsum

fixed_sum_divisors(0) # sum of 0 = 0
fixed_sum_divisors(3) # Should sum of 1 = 1
fixed_sum_divisors(36) # Should sum of 1+2+3+4+6+9+12+18 = 55
fixed_sum_divisors(102) # Should be sum of 2+3+6+17+34+51 = 144 (wrong sum)

Output:

0 ==> [] ==> 0
3 ==> [1] ==> 1
36 ==> [18, 12, 9, 6, 4, 3, 2, 1] ==> 55
102 ==> [51, 34, 17, 6, 3, 2, 1] ==> 114  # your 144 is wrongly summed up

Answer 4

Iterating over a range like that isn't very efficient because once you get past the square root of n, any further tests are redundant.

I found a splendid implementation of a function that derives all factors of a given number here

I made a small modification to it and adapted it to this particular problem leading to:

from functools import reduce

def factors(n):
    return set(reduce(list.__add__,
                ([i, n//i] for i in range(1, int(n**0.5)+1, n % 2 + 1) if not n % i)))

def sum_divisors(n):
    return sum(factors(n)) - n
    
print(sum_divisors(102))

Output:

114

...and not 144 as suggested in the OP's question.

Credit:

@agf

Answer 5

When you do

for i in range(n ,0 , -1):
if n % i == 0:
  sumlist.append(i)
  n -= 1

n is being included. Thus,

print(sum_divisors(3)) # Should sum of 1 = 1

will also include the 3 in the check, giving a result greater than what you want.

For some thing closer to what you want:

for i in range(n-1,0 , -1):
if n % i == 0:
  sumlist.append(i)
 # n -= 1  This should be removed

Answer 6

You should start your range ( in line 3 ) from n-1 , and then do it again whilst removing unnecessary code ( as mentioned in above answers ) .

Try this :

def sum_divisor(num): # Define your function here
    factors_list = [] # Create an empty list here
    for i in range(1,num): # Start your range from 1 and take it to n (beacuse the range will go on till n-1)
        if num % i == 0: # Check if it's divisible
            factors_list.append(i) # Append , if it's true

    total = sum(factors_list) # Add all the factors given in the list
    print(total) # Print 'em

input = int(input("Enter your data : "))
sum_divisor(input)

Answer 7

As always when you work with divisors, you can speed up things a lot by just looping up to the square root of n. When you find a divisor i lower than the square root you know n // i is a divisor too.

This leaves us with some special cases to handle - especially so since you want to consider 1 as a divisor while not considering n itself. See this code:

def sum_divisors_sqrt(n):
    if n < 1:
        return 0
    sum_divs = [1]
    sqrt_n = int(sqrt(n))
    if n % sqrt_n == 0:
        sum_divs.append(sqrt_n)
    for i in range(2, sqrt_n):
        if n % i == 0:
            sum_divs.append(i)
            sum_divs.append(n//i)
    return sum(sum_divs)

On my PC this is 20x faster for n = 1000, 90x faster for n = 10000