Nice quickstart guide! Nothing much more than small details to comment on :).
I am not sure of the common practices for comments on PR, I will expect that commenting here is a good option (the other solution I thought about is changing the guide and do a PR on the PR, but it seems like an infinite loop...). Tell me if I should do differently :).

• Item 2.a, there is a typo compared to the image: you speak about SST but made a box around the SSH
• Item 3, maybe add a short sentence on the usage of the boundary argument, e.g.
  The boundary condition is used for operations at the boundary points, when an extra point needs to be added. Different options are possible ('fill' this extra values with a certain number, 'extend' to the nearest value or 'interpolate' linearly using the 2 nearest points). Attention, this boundary condition is used to create an extra point before running the operation, it is not used to fill the dataset afterward for points that could not be computed.
  Maybe this description would suit more on another part of the doc, but this boundary condition was also something I struggled with at the beginning.

Apart this quickstart guide, would it be useful if the 1st example was partially model-agnostic? Or maybe easier to do, the simple MITgcm example could describe more the grid and the scale factors / metrics so the grid creation is understandable by more people. This would be a logical continuity of the quickstart guide, where the notions are applied.

Dear @rcaneill , thanks for reviewing!

To your question: It actually works really nicely (especially for text based files) to make a PR to a PR. You just have to edit the file in question on github (no cloning needed), and it will give you the option of creating a new PR. This will show up as a suggestion in the discussion section. If you like please try it for the typo.

I came across the issue with the boundary as well yesterday. We never really mention it. Only in a convoluted way in the setup code examples. We need to add a more detailed section on 'boundary' and 'fill' in 'Simple Grids' asap!(cc @rabernat , @dcherian ).

I was thinking of making a completely `synthetic example at the end (e.g. make up some data, create a grid, metrics and then apply some operator). It seems that this would be in your interest too, so let me add that.

To your question: It actually works really nicely (especially for text based files) to make a PR to a PR. You just have to edit the file in question on github (no cloning needed), and it will give you the option of creating a new PR. This will show up as a suggestion in the discussion section. If you like please try it for the typo.

Amazing! I try it now!

Thanks a ton for the comments @rcaneill and @rabernat. Ill try to make the example at the end next week and then we can merge this.

Hi @jbusecke. Just pinging on this. Are you ready for another review? Would be great to get this merged.

Have put aside today for the OMZ paper completely. Ill try to finish this tomorrow or Friday!

@jbusecke Responding here to your comment/request on #291.

The quick start guide seems like a very good start! It definitely makes understanding how xgcm works on a high level much much easier. I would suggest that, after those 4 steps are done and you put it all together with pseudo code, you actually do all those steps for a simple model and explain the options and choices. Some things are very straightforward to understand, but I think for others some work is needed to make it clear.

At least for me, here's how it goes:

• The grid/node nomenclature is pretty easy to understand (left, center, outer, etc.; the cartoon you guys use is really all that's needed), as is toggling the periodic topology options. So I think no need to expand too much on that.
• The boundary conditions are a bit harder to understand. I know they should roughly map to Neumann/Dirichlet boundary conditions, but it's not super clear exactly how to translate from my model's BC to what xgcm needs to (almost?) exactly reproduce it.
  • Also, this may be a separate issue, but I find it weird that I can set a BC for a periodic axis. It makes me a little confused on how those work.
• The metrics for me are definitely the hardest to understand. (In fact I still haven't understood exactly what they are measuring.) So in this part I'm afraid some work has to be done to figure out how to design a cartoon and an example that explain exaclty what they are and how to create them for any model output. Otherwise the user might put something that works but is incorrect and will get wrong answers from then on.

Hope this helps! And keep up the good work

Thank you for taking the time to go through this @tomchor. This is extremely helpful feedback.

Some quick notes:

The boundary conditions are a bit harder to understand.

💯 agree. In fact we barely describe the boundary conditions at all in the docs (see also #273). I am not sure how much detail should go into the quickstart guide vs a separate section. I personally think a lot of these details should go into a more detailed section.

Also, this may be a separate issue, but I find it weird that I can set a BC for a periodic axis. It makes me a little confused on how those work.

Yeah, this is very confusing, and should definitely be remedied in the future. This will require some rather big changes and a longer deprecation cycle thought (see #195). How about for now I add a cautionary note?

The metrics for me are definitely the hardest to understand. (In fact I still haven't understood exactly what they are measuring.) So in this part I'm afraid some work has to be done to figure out how to design a cartoon and an example that explain exaclty what they are and how to create them for any model output. Otherwise the user might put something that works but is incorrect and will get wrong answers from then on.

Ok I hear that and will try to spend some time to make this more clear. In short, metrics are quantities that relate the logical rectangular grid (or nd-array) to actual physical space, like distances between cell points, cell volume, and area, etc. How are these kinds of parameters given in the LES world? Perhaps I can draw some inspiration from there to formulate something more general.

Again many thanks for using xgcm and providing this great feedback!

Yeah, this is very confusing, and should definitely be remedied in the future. This will require some rather big changes and a longer deprecation cycle thought (see #195). How about for now I add a cautionary note?

Yeah, a cautionary note would definitely help. Also, adding a boundary="periodic" is also something I was gonna suggest. Seems like the easy way out.

Ok I hear that and will try to spend some time to make this more clear. In short, metrics are quantities that relate the logical rectangular grid (or nd-array) to actual physical space, like distances between cell points, cell volume, and area, etc.

I think what metrics represent, is pretty well-explained. I think what is confusing is how exactly to represent that. Because there are many different sets of metrics that you can come up with that fully represent the geometry of a grid.

How are these kinds of parameters given in the LES world?

For finite volume codes like Oceananigans the usual approach is to use a rectilinear C-grid. So it's even simpler than GCMs, that have to account for curvature, etc. Other types of code (the usual ones mix spectral in the horizontal and finite differences in the vertical) have a slightly different grid where the only variable on cell faces is the vertical velocity. Fully spectral codes (like Dedalus) use only cell centers I think. Again, LES grids should be actually easier in general.

This pull request has been marked 'stale' due to lack of recent activity. If there is no further activity, the pull request will be closed in another 30 days. Thank you for your contribution!

This pull request has been closed due to inactivity. If you feel this is in error, please reopen the pull request.

Hello @jbusecke, this is a very nice part of the doc, but that seems stopped for a while... I would be happy to help to finish it, if help is needed for anything!
If so, please let me know with a list of tasks to write :)

Hey @rcaneill, thanks for the ping on this. I could indeed need some help on this. I am working with @TomNicholas on a larger internal refactor, which will fundamentally change some of the terminology regarding padding and boundary. In short we will rename the current 'boundary' to 'padding' (this is only concerned with the array boundaries which do not mean anything in terms of physical boundaries in the domain! This was always a bit confusing). Once we have merged the new internals (https://github.com/xgcm/xgcm/tree/grid_ufunc_refactor_project), we can actually deal with internal boundary conditions (#324).

I think this distinction and explanation should be included in the quick start guide, but that might not mean that this should not get finished/merged before that.

I think the checklist at the top is still quite up to date. The most important thing to include (as @tomchor mentioned) is how to include metrics I think.

It would be amazing if you could take a shot at adding the missing items. Happy to review and also happy to fill in the boundary discussion (perhaps for now with a warning/note until properly implemented?).

Oh yes I saw the internal refactoring, I really look forward to using the grid_ufuncs!
So I will start with the metrics, and then add a work about boundary / periodic, with a warning note.

How should I work on this PR, is it possible for me to clone the jbusecke:quickstart_guide branch, and do a PR on it? Or is it easier if I only work via github and propose changes (does not seem very convenient)

Yeah lets go via the github route. I propose:

• You fork the repo (probably already done?)

• Define my fork as remote e.g. git remote add jbusecke ...

• Then fetch this branch, checkout from the current state under a new name

• Push and create a PR from your changes to this branch (I suggest doing this with some minimal changes, so we can see if this works haha).

Actually let me try to resolve the issue in this branch first!

Ok I think that did the trick. Feel free to fetch this state. And thanks a lot for helping here @rcaneill!

I opened a PR on your fork, so I guess that if you merge, it will appear here.
I can run the tests locally, and tell you whenever it is ready to merge here I guess

Perfect!

Hello,
I ping here in case someone has time to review my work on Julius fork: jbusecke#2