5. Factorial, code not working :(


#1

Hello,

So I'm trying the factorial exercise and I want to know why my particular isn't working.

I have:

def factorial(x):
[tab]if x==0 or x==1:
[tab][tab]return 1
[tab]else:
[tab][tab]while x>1:
[tab][tab][tab]product = x*(x-1)
[tab][tab][tab]x-=1
[tab][tab][tab]return product

But it breaks after x=4 :confused: Not sure why :frowning:


#2
def factorial(x):
[tab]if x==0 or x==1:
[tab][tab]return 1
[tab]else:
[tab][tab]while x>1:
[tab][tab][tab]product = x*(x-1)
[tab][tab][tab]x-=1
[tab][tab][tab]return product

Use three back-ticks before and after code block to preserve format.

When creating indents, use four spaces, not tab.

def factorial(x):
    if x==0 or x==1:
        return 1
    else:
        while x>1:
            product = x*(x-1)
            x-=1
            return product

The above has an extra indent on the return statement.

def factorial(x):
    if x==0 or x==1:
        return 1
    else:
        while x>1:
            product = x*(x-1)
            x-=1
        return product

Still have to test it, but the indentation should be fixed, now.


#3

This line is going to give you a problem:

product = x*(x-1)

It is a diminishing value. Consider

 10 * 9
  9 * 8
  8 * 7

and so on. The final value will always be incorrect. Let's try something a little bit different that doesn't diminish...

    product = 1
    while x > 1:
        product *= x
        x -= 1
    return product

#4

This method is only slightly different

def factorial(n):
    n = abs(n)
    f = n
    while n > 1:
        n -= 1
        f *= n
    return f
print factorial(10)

#5

Another different method: very simple

def factorial(x):

x= abs(x)
s=1
r= []
for i in range(1,x+1):
    r.append(i)
print r

for k in r:
        s=k*s
return s
print s

#6

Of course! I knew it had to be something to do with that line! Thank you so much! :smiley:


#8

def factorial(x):
    if x == 0 or x == 1:
        return 1
    else:
        return (x * factorial(x - 1))

#9

so nice and simple! gj


#10

Recursion is very deceiving. It looks simple but it is anything but from the computer's standpoint, being very resource intensive. Yes, it is important to understand recursive functions, how they work and how to write them. Just don't be fooled into thinking them simple. There is a reason they are not discussed in these introductory tracks. In most cases, we can write an algorithm that is comparably fast, or faster, and in most cases more efficient.


#11

Why do we need second line (n=abs(n)), is it really essential?


#12

That step ensures that n is a positive number.


#13

@mtf

Take a gander at this post I compared several factorial functions to see how fast they were.

Just for fun @jirapong_p I will add your time to this post, I had to change the max recursion depth because of your code though.

CODE:

import sys
import cProfile
sys.setrecursionlimit(20000)

def factorial(x):
    if x == 0 or x == 1:
        return 1
    else:
        return (x * factorial(x - 1))

def cf_range(num_range):
    """
    This function saves the factorial to file. It also calls the function
     ranging from 1 to the given number. So all factorials to the number are
     saved to file. The computer has problems displaying any number
     the system can not handle.
    :param num_range: Range of numbers to be factored
    """
    with open("factorial_of_%s.txt" % str(num_range), "w+") as file:
        for number in range(1, num_range + 1):
            file.write(str(factorial(number)) + "\n")

cProfile.run("cf_range(5000)")

OUTPUT: @4500 Count file 29,708kb

         10145259 function calls (22509 primitive calls) in 50.292 seconds

   Ordered by: standard name

   ncalls  tottime  percall  cumtime  percall filename:lineno(function)
10127250/4500   24.196    0.000   24.196    0.005 <pyshell#25>:1(factorial)
        1   25.548   25.548   50.292   50.292 <pyshell#27>:1(cf_range)
        1    0.000    0.000   50.292   50.292 <string>:1(<module>)
        1    0.000    0.000    0.000    0.000 _bootlocale.py:11(getpreferredencoding)
        1    0.000    0.000    0.000    0.000 codecs.py:183(__init__)
        1    0.000    0.000    0.000    0.000 codecs.py:257(__init__)
     4500    0.001    0.000    0.001    0.000 codecs.py:273(reset)
     4500    0.010    0.000    0.338    0.000 cp1252.py:18(encode)
        1    0.000    0.000    0.000    0.000 {built-in method _getdefaultlocale}
     4500    0.328    0.000    0.328    0.000 {built-in method charmap_encode}
        1    0.000    0.000   50.292   50.292 {built-in method exec}
        1    0.000    0.000    0.000    0.000 {built-in method open}
        1    0.000    0.000    0.000    0.000 {method 'disable' of '_lsprof.Profiler' objects}
     4500    0.208    0.000    0.548    0.000 {method 'write' of '_io.TextIOWrapper' objects}

As you can see you had 10 million function calls with your 4500 recursive function calls in 50.3 secs which vs my code at

So as you can see the larger the number gets the more time effective my code gets, while my code

Is quite a bit faster than yours even though you have a very succinct code. If you do not get some of my code ask away and I will try to explain it best as I am able.


#16

this is shorter but not really simpler

import operator
def factorial(x):
    z=[]
    y=0
    for i in range(x):
        y += 1
        z.append(y)
    return reduce(operator.mul, z)

#17

Why not just,

for y in range(x):

?

That omits,

    y = 0

and

        y += 1

from the listing.


#18

lol I didn't think of that but you would need to edit the z.append(y) to be z.append(y+1). Remember python always starts counting at 0 and if you multiply by 0 you get 0.

heres the updated code.

import operator
def factorial(x):
    z=[]
    for y in range(x):
        z.append(y+1)
    return reduce(operator.mul, z)

#19

Right you are. What was I thinking? Good catch.


#20

recursive function ftw.


#21

I'm trying to visualize this....is this kind of like Inception (a function within a function within a function)? So you keep stepping into another factorial function ( in -1 increments) and once you return 1 it reiterates backwards and performs the multiplication? If anyone could explain the exact steps here, it would be much appreciated.


#22

Instead of using recursion we can just use a lambda and get the same result on one line.

from functools import reduce
def factorial(number):
    return reduce(lambda x, y: x*y, range(1, number))

There are a few other ways to solve this problem but all the answers so far are defiantly good ones.


#23

This is so elegant. Thank you.