Jupyter notebooks #294
Jupyter notebooks #294
Conversation
|
Hopefully changes in #297 fix the above issues |
…_fixes Fixed failing notebook tests, updated CI, made tests DRYer
There was a problem hiding this comment.
I think this looks really nice and I'm really excited for us to have these in Gusto. Thanks so much for doing all of this.
I have a few pernickety comments about style of some particular comments and things we can probably omit -- if you disagree with anything please feel free to ignore it. The main thing that I think would be nice is to add lint checking, which I suspect may highlight some more issues.
I've only looked at the first notebook -- I thought I'd come back to that in case you want to make any changes having used the lint checker.
I was also wondering whether we want to include the mountain notebook? Some of the routines in there aren't that obvious so it might be quite challenging as an introduction. Maybe there's something important in that case that I'm missing though that is helpful!
My final suggestion is about the names of the notebooks. I think the first one is great, the second one could do with some capitalisation to make it look consistent (i.e. Book_2_Gravity_Wave_Vertical_Slice) and finally I think the final one might be best as Book_3_Lowest_Order_Rising_Bubble (or book 4 if you keep the mountain!)
Makefile
Outdated
| @@ -10,18 +10,24 @@ lint: | |||
| @echo " Linting gusto plotting scripts" | |||
| @python3 -m flake8 plotting | |||
There was a problem hiding this comment.
I think it would be nice to add the notebooks to the lint checker here, I think just by adding:
@echo " Linting gusto jupyter notebooks"
@python3 -m flake8 jupyter_notebooks
I expect when you run make lint it will then throw up a bunch of things to change in the notebooks though! But that will help make sure the code is consistent everywhere. One of the main things will be to make sure that we leave a space between the # at the start of a comment and the text after it.
| "metadata": {}, | ||
| "source": [ | ||
| "This notebook provides an introduction to Gusto. We will go through the example of the Williamson 2 test case to demonstrate how to set up the problem and run it. This comes from the paper: \\\n", | ||
| "David L. Williamson, John B. Drake, James J. Hack, Rüdiger Jakob, Paul N. Swarztrauber,\n", |
There was a problem hiding this comment.
I think it would be OK to give the reference in a shorter form since we are giving the link to it, i.e.:
"This is the second test from Williamson et al (1992), "
| "\n", | ||
| "The parameters we need to specify are the Coriolis force, $f$, gravitational constant, $g$, and mean depth, $H$.\n", | ||
| "$\\newline$\n", | ||
| "We can neglect boundary conditions, due to solving on a spherical domain. \\ \n", |
There was a problem hiding this comment.
There is a rogue backslash somewhere around here
| "source": [ | ||
| "The Williamson 2 test case solves the shallow water equations. This corresponds to the following set of momentum and continuity equations, for a velocity $\\textbf{u}$ and free surface, $\\eta$:\n", | ||
| "$$\\textbf{u}_t + f \\textbf{u}^{\\perp} + g \\nabla \\eta + (\\textbf{u} \\cdot \\nabla) \\textbf{u} = 0$$\n", | ||
| "$$\\eta_t + H(\\nabla \\cdot \\textbf{u}) + \\nabla [\\textbf{u} (\\eta - b)] = 0$$\n", |
There was a problem hiding this comment.
I have a very petty suggestion for how we write the shallow-water equations. I prefer to see the transport term (u.grad)u immediately after the time derivative in the momentum equation (so that you can easily see that together these are the material derivative). For the continuity equation, can we just write
\\eta_t + \\nabla \\cdot [(\\eta - b) \\textbf{u}] = 0?
Please feel free to ignore if you disagree!
There was a problem hiding this comment.
Yes, I agree with grouping the terms in that way and I also think we should try to be consistent with the names in the code - i.e. we use D for the prognostic variable, where D=H+\eta-b.
| "The parameters we need to specify are the Coriolis force, $f$, gravitational constant, $g$, and mean depth, $H$.\n", | ||
| "$\\newline$\n", | ||
| "We can neglect boundary conditions, due to solving on a spherical domain. \\ \n", | ||
| "Williamson 2 is called 'Global Steady State Nonlinear Zonal Geostrophic Flow', so the solution should not change over time." |
There was a problem hiding this comment.
Maybe we could rewrite this as "The second Williamson test case models zonal geostrophic flow. This is a steady-state solution of the shallow-water equations, so the true solution does not change with time."
| "source": [ | ||
| "from gusto import *\n", | ||
| "from firedrake import IcosahedralSphereMesh, SpatialCoordinate, as_vector\n", | ||
| "from math import pi\n", |
There was a problem hiding this comment.
I wonder if we could actually just import pi from firedrake? I think we also might not need to do import sys
There was a problem hiding this comment.
yes, this is probably highlighting that the examples need a bit more tidying...
| "metadata": {}, | ||
| "outputs": [], | ||
| "source": [ | ||
| "output = OutputParameters(dirname=\"sw_W2_ref%s_dt%s\" % (ref_level, dt),\n", |
There was a problem hiding this comment.
We actually can probably give a clearer directory name -- maybe just:
dirname="williamson_2_intro"
| "source": [ | ||
| "We are now ready to specify the initial conditions.\n", | ||
| "Due to our choice of function spaces for the velocity and depth, the intialisations of each variable use projection \n", | ||
| "and interpolation operations respectively. Williamson 2 specifies initial conditions for a 'Global Steady State Non-linear Zonal Geostrophic Zone' (do I need this detail?). \n", |
There was a problem hiding this comment.
I think you don't need this detail -- we've explained the initial condition at the top so I think it's OK to leave this out
| "and interpolation operations respectively. Williamson 2 specifies initial conditions for a 'Global Steady State Non-linear Zonal Geostrophic Zone' (do I need this detail?). \n", | ||
| "$$ \\textbf{u}_0 = \\frac{u_{max}}{R} [-y,x,0] $$\n", | ||
| "$$ h_0 = H - \\frac{\\Omega u_{max} z^2}{g R} $$\n", | ||
| "We only require a two-dimensional velocity, so the vertical component is specified to be zero. (Jemma/Tom question: Why is $R/R^2$ used in the code)???" |
There was a problem hiding this comment.
I think we are embedded in a 3D space so we are specifying all three of the Cartesian components of the velocity. I don't think we need the R/R^2 so maybe you can cancel one out! Presumably the was originally done to make it clear that the final part is sin^2(lat)?
| "metadata": {}, | ||
| "outputs": [], | ||
| "source": [ | ||
| "#Now, construct the time-stepper. We will firstly use a semi-implicit (?) approach.\n", |
There was a problem hiding this comment.
Yes it's semi-implicit!
|
I suggested the mountain as it demonstrates how to deform the mesh - so maybe not an introductory notebook but a more advanced one! |
|
Thanks Tim, these are a great start to our notebook resources! |
Ah OK! Maybe that can be the fourth then? And maybe it should be called Book_4_Flow_Over_A_Mountain |
|
Hi Tom and Jemma, thanks for the feedback! I've made the changes you've mentioned and have added the notebooks to the lint checker, so will see what that comes back with. |
Merge branch 'main' of https://github.com/firedrakeproject/gusto into tim_FML_notebooks
…ct/gusto into tim_FML_notebooks
… tim_FML_notebooks
There was a problem hiding this comment.
Does this file need deleting?
| --timeout=1200 \ | ||
| --timeout-method=thread \ | ||
| -o faulthandler_timeout=1260 \ | ||
| --nbval-lax \ |
There was a problem hiding this comment.
| --nbval-lax \ | |
| --nbval \ |
When you're happy with the state of the notebooks, you can drop the lax and the test will check that the output doesn't change. Use # NBVAL_IGNORE_OUTPUT for specific cells where this is undesirable, see these docs.
Added a directory with four example notebooks to demonstrate the functionality of Gusto.