Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tim fml notebooks #294

Open
wants to merge 51 commits into
base: main
Choose a base branch
from
Open

Tim fml notebooks #294

wants to merge 51 commits into from

Conversation

ta440
Copy link
Collaborator

@ta440 ta440 commented Aug 23, 2022

Added a directory with four example notebooks to demonstrate the functionality of Gusto.

@JDBetteridge
Copy link
Member

Hopefully changes in #297 fix the above issues

…_fixes

Fixed failing notebook tests, updated CI, made tests DRYer
Copy link
Contributor

@tommbendall tommbendall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!)

@@ -10,18 +10,24 @@ lint:
@echo " Linting gusto plotting scripts"
@python3 -m flake8 plotting
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we could actually just import pi from firedrake? I think we also might not need to do import sys

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)???"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it's semi-implicit!

@jshipton
Copy link
Contributor

I suggested the mountain as it demonstrates how to deform the mesh - so maybe not an introductory notebook but a more advanced one!

@jshipton
Copy link
Contributor

Thanks Tim, these are a great start to our notebook resources!

@tommbendall
Copy link
Contributor

I suggested the mountain as it demonstrates how to deform the mesh - so maybe not an introductory notebook but a more advanced one!

Ah OK! Maybe that can be the fourth then? And maybe it should be called Book_4_Flow_Over_A_Mountain

@ta440
Copy link
Collaborator Author

ta440 commented Aug 30, 2022

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.
Let me know when you change the name of the time-stepper from 'Crank-Nicolson', as I'll need to update the notebooks to compensate.

Copy link
Contributor

@tommbendall tommbendall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again Tim for all your work on this! I've been through all the notebooks now and think it's looking good. I've left what seems to be a lot of comments but I think they should hopefully be very minor -- it's mainly neatening a few things that are still leftover and answering your questions.

The most major thing is that I'm wondering if it'd actually be clearer to reome the plotting code from the second notebook, and additionally the point data file?

"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 + (\\textbf{u} \\cdot \\nabla) \\textbf{u} + f \\textbf{u}^{\\perp} + g \\nabla \\eta = 0$$\n",
"$$\\eta_t + \\nabla \\cdot [(\\eta - b) \\textbf{u}] = 0$$\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry to be a pain since you already changed this! I'm wondering now if we should make sure that this matches the equations used in the code. I think that means doing "depth $D$" and the equations becoming:
$\textbf{u}_t + (\textbf{u} \cdot \nabla) \textbf{u} + f \textbf{u}^{\perp} + g \nabla(D+b) = 0$,
$D_t + \nabla \cdot (D \textbf{u}) = 0$

"metadata": {},
"source": [
"We can specify which diagnostics we wish to record over a simulation. The list of avaliable diagnostics can be found in the gusto source code: https://github.com/firedrakeproject/gusto/blob/main/gusto/diagnostics.py \\\n",
"We pass these diagnostics into the State function. This will initialise all the necessary components of the test case. Gusto will update this state after each iteration of the chosen time-stepper."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It could be nice to use code formatting for State to highlight that it's a code object. I guess this is the backwards apostrophes?

"id": "80f8c00a",
"metadata": {},
"source": [
"A spatially varying Coriolis force is defined over the sphere. We then can set-up our form of the shallow water equations by passing the state and Coriolis parameter into the ShallowWaterEquations() function. We additionally specify the function space (BDM) and degree (1) that we want to use with the Compatiable Finite Element method. "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As above, it could be nice to use code formatting for the ShallowWaterEquations() phrase to highlight that it's a code object.

"id": "1e1ae651",
"metadata": {},
"source": [
"We can specify which diagnostics we wish to record over a simulation. The list of avaliable diagnostics can be found in the gusto source code: https://github.com/firedrakeproject/gusto/blob/main/gusto/diagnostics.py \\\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the github page there is a rogue backslash after the hyperlink -- is it possible to remove this?

"id": "2b518722",
"metadata": {},
"source": [
"After the simulation is completed, you should see that there is now a 'results' directory, with another directory holding the output. The output at the specified times is saved in a .vtu format that can be viewed using Paraview. There are five files, corresponding to the specified dump times (field_output1.vtu, field_output2.vtu, etc.), as well as one for the initial conditions (field_output0.vtu). Paraview can be downloaded from the following link: https://www.paraview.org/ \\"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the github page there is a rogue backslash after the hyperlink -- is it possible to remove this? No worries if not or if it doesn't appear normally.

"id": "3ec4b5fe",
"metadata": {},
"source": [
"# Notebook 3: Mountain Wave Test Case"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose this is notebook 4 now -- sorry!

"id": "eb79138f",
"metadata": {},
"source": [
"This notebook will go through an example of a hydrostatic flow over a mountain. This test case is outlined in the paper: Melvin, T., Dubal, M., Wood, N., Staniforth, A., & Zerroukat, M. (2010). An inherently mass‐conserving iterative semi‐implicit semi‐Lagrangian discretization of the non‐hydrostatic vertical‐slice equations. Quarterly Journal of the Royal Meteorological Society: A journal of the atmospheric sciences, applied meteorology and physical oceanography, 136(648), 799-814."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again we can probably shorten this citation and give the link: https://doi.org/10.1002/qj.603

"id": "2fb1fb2d",
"metadata": {},
"source": [
"We now invoke the smooth_z variable, I assume to smooth out the vertical profile somehow (do we want an explanation of this?? We then apply this smoothing to the defined mesh."
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I actually think it's clearest if we remove the smoother_z flag? But we can still use the smoother z profile. The description here could then be something like: "Now we transform the mesh to follow the defined terrain. A smoothing is applied in the vertical direction."

"outputs": [],
"source": [
"supg = True\n",
"if supg:\n",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As in the second book we should probably just pick to use supg and get rid of the flag here

"id": "10e50cf7",
"metadata": {},
"source": [
"What other feature do we want to demonstrate in this notebook?"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that's all good!

@ta440
Copy link
Collaborator Author

ta440 commented Sep 29, 2022 via email

@tommbendall tommbendall added the admin Involves admin of the repository or associated website label Aug 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
admin Involves admin of the repository or associated website
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants