Exercise 6 ready for review

[valentineap]

'For' loops

https://github.com/valentineap/ComputationalGeoscienceCourse/tree/master/Practicals/Exercise%206%20-%20More%20loops

[oscarbranson]

Another possible addition - list comprehension? And then expansion to tuple/dict comprehension? Happy to put this in, if you think it would be useful.

[oscarbranson]

Just realised that list comprehension is already in there... still, pointing out that you can do similar with tuples:

(i for i in array)

or dicts:

{k: v for k, v in zip(keys, values}

might be helpful?

[charlesll]

On my side, I'm not convinced with the "factorial" calculation... I do not have a good result trying this because I dislike factorials and I can't really understand how to properly code the 2k+1 factorial inside the loop... What is the solution?

Maybe you want to consider this, and put the Leibniz formula instead? It still is slower to converge than the other formula, but avoids the issue of coding the factorials... I have a notebook with the proper Markdown if you want. (but maybe it is just me that strongly dislike factorials, always!)

[valentineap]

@charlesll I thought the factorial could be done something like this:

def fact(n):
    f = 1
    for i in range(2,n+1):
        f*=i
    return f

[The canonical approach is to use recursion:

def fact(n):
    if n==0:
        return 1
    else:
        return n*fact(n-1)

but I suspect this is a little clever to expect the students to invent...]

Why are you considering this difficult? I'm open to changing...

Note that the formulae for pi converge in about 10-20 terms, you don't need something that is able to calculate 10000000! robustly. Perhaps we should add a note to this effect...

[valentineap]

@oscarbranson Yes, I'll add a note on tuple/dict comprehensions - good spot.

[oscarbranson]

@charlesll @valentineap

I thought it was pretty OK, and a good way of introducing nested loops.

For what it's worth, I did it like this:

# Try it here!
pi = 0
niter = 25

for k in range(niter):
    kfac = 1
    for i in range(1, k+1):
        kfac *= i

    denom = 1
    for i in range(1, (2 * k + 1) + 1):
        denom *= i

    pi += 2**k * kfac**2 / denom

    print(2 * pi)

pi *= 2

No extra functions, but rather inelegant!?

[valentineap]

@oscarbranson @charlesll I've added the dictionary and tuple comprehensions, and added a couple of sentences to give a hint on how to compute the factorials. I hope this addresses any concern.

Another one ready for @rmcgirr94 .

[charlesll]

@valentineap OK it's actually fine.

I was a bit brained-washed yesterday afternoon and I did not noticed I had a problem upon initializing the factorial calculation, fact(0) was returning 0 and that was messing up the calculation. Now I get it.

(Actually, I really like the idea of recursion!)

Good to go for me!

[rebecca-mcgirr]

Finished testing, I haven't made any changes. My only comment is on the example of list comprehension with tuples: a = (i**2 for i in range(3,13,2)) returning a outputs a memory address I'm not sure what the resolution is or how tuples work re list comprehension.

[valentineap]

@rmcgirr94 Good spot. It should have been: a = tuple(i**2 for i in range(3,13,2)) -- apparently tuples get a special syntax... this is the annoying thing about Python...

Now fixed. Please check and close this issue if you're happy.

[rebecca-mcgirr]

Thanks @valentineap, closing this issue.

[oscarbranson]

This is my bad - I was thinking Python 2 when I suggested tuple comprehension.

In Python 3 (a for a in array) produces a generator, not a tuple. Prepending tuple turns it into one by evaluating the entire generator, but kind of defeats the point.

Maybe introduce generators here instead?