Comments on notebooks

[nicoguaro]

For openjournals/jose-reviews#21.

[nicoguaro]

01_Step_1

• ipython notebook >> jupyter notebook

[nicoguaro]

02_Step_2

• Cell [1],
  import numpy #we're importing numpy and calling it np locally
  NumPy is called numpy and not np. Same goes for matplotlib.pyplot.

[nicoguaro]

05_Step_4

• Cell [3], I think that it would be more consistent to use sympy.pi instead of numpy.py
phi = (sympy.exp(-(x - 4 * t)**2 / (4 * nu * (t + 1))) + sympy.exp(-(x - 4 * t - 2 * numpy.pi)**2 / (4 * nu * (t + 1)))) >> phi = (sympy.exp(-(x - 4 * t)**2 / (4 * nu * (t + 1))) + sympy.exp(-(x - 4 * t - 2 * sympy.pi)**2 / (4 * nu * (t + 1))))

[nicoguaro]

06_Array_Operations_with_NumPy

• After Cell [6]: it might be useful to mention that the running time depends on the computer. For some people it might be obvious, but that's not always the case.

[nicoguaro]

13_Step_10

• Before cell [1], I would change b_{i,j}=-100$ at $i=nx*3/4, j=3/4 ny for b_{i,j}=-100$ at $i=3/4 nx, j=3/4 ny

[nicoguaro]

14_Optimizing_Loops_with_Numba

• The notebook mixes spaces and tabs.

• laplace2d functions mix the alias for NumPy, using numpy and np. Due to this the notebook does not work, the following is the returned error

  NameError: name 'np' is not defined

Clearly the NumPy + Numba combination is the fastest, but the ability to quickly optimize code with nested loops can also come in very handy in certain applications.

This is not the case in my computer. I have 91.1 µs ± 40.2 µs in the NumPy + Numba case and 62.5 µs ± 30.8 µs in the Vanilla + Numba.

[nicoguaro]

15_Step_11

It might be a good idea to consider streamplots besides glyphs for the vector visualization. It suffices to change

pyplot.quiver(X, Y, u, v)

for

pyplot.streamplot(X, Y, u, v)

For some vector fields this is easier to understand, and introduces another visualization technique.

This comment is a matter of taste, so I understand if the authors don't consider it needed.

[nicoguaro]

17_NumbaPro

• The extension for this notebook has a .bak appended.

• Cell [1] returns the following warning

  numbapro:1: ImportWarning: The numbapro package is deprecated in favour of the accelerate package. Please update your code to use equivalent functions from accelerate.

• Cell [1] returns the following error

  from numbapro import autojit, cuda, jit, float

• Cell [5] returns the following error

  DeprecationError: Deprecated keyword argument argtypes. Signatures should be passed as the first positional argument.

• The notebook uses Python 2.x printing.

[nicoguaro]

18_Burgers_equation

• Cell [6] returns the following error

  from JSAnimation.IPython_display import display_animation

  JSAnimation is not mentioned as a dependency. A workaround that is compatible with FuncAnimation```is to use the IPython magic %matplotlib notebookinstead of%matplotlib inline`

• Markdown titles are missing a space between pound sign (#) and titles

[nicoguaro]

19_Odd_Even_Decoupling

The notebook depends on a folder with videos that is missing from the repository.

[gforsyth]

Notebooks 17, 18 and 19 were never really edited or tested -- at this point my inclination is to just delete them (or move them to a subdirectory). Especially since numbapro no longer exists and the numba stuff in general is all very out of date.

Thoughts, @labarba ?

[gforsyth]

@labarba -- I've pushed up individual commits addressing the comments on notebooks 1, 2, 5, 6 and 13, respectively, in #46 . If you disagree with any of the fixes just let me know and I can pull out the individual commits in question.

I like the suggestion about using streamplot (which, if I recall, was less reliable back when we wrote these).

Here's the existing quiver plot:

And here's the same plot using streamplot instead.

Preference?

[labarba]

I don't know ... I like that the quiver plot shows magnitude of velocity with the arrow length.

[labarba]

Maybe include them both? 🤔

[labarba]

Regarding notebooks 17, 18, 19 — these are drafts, really… we could move them to a WiP folder?

[UPDATE] No, I think we should remove these notebooks from this repo. They could be the beginning of a new module, but "CFD Python" is round and neat up to lesson 12.

[gforsyth]

I removed notebooks 17-19 and also the very out of date Numba notebook (14). And added an additional plot to step 11 with the streamplot after the first two quiver plots. All of this in #46