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

WIP: Quickstart Guide for the docs #271

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

jbusecke
Copy link
Contributor

@jbusecke jbusecke commented Oct 13, 2020

@rcaneill
Copy link
Contributor

rcaneill commented Oct 13, 2020

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.

@jbusecke
Copy link
Contributor Author

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.

@rcaneill
Copy link
Contributor

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!

doc/quickstart.rst Outdated Show resolved Hide resolved
doc/quickstart.rst Outdated Show resolved Hide resolved
Copy link
Contributor

@rabernat rabernat left a comment

Choose a reason for hiding this comment

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

This is a great start.

doc/quickstart.rst Outdated Show resolved Hide resolved
doc/quickstart.rst Outdated Show resolved Hide resolved
doc/quickstart.rst Outdated Show resolved Hide resolved
doc/quickstart.rst Outdated Show resolved Hide resolved
doc/quickstart.rst Outdated Show resolved Hide resolved
jbusecke and others added 7 commits October 16, 2020 19:57
Co-authored-by: Romain Caneill <romain.caneill@ens-lyon.org>
Co-authored-by: Romain Caneill <romain.caneill@ens-lyon.org>
Co-authored-by: Ryan Abernathey <ryan.abernathey@gmail.com>
Co-authored-by: Ryan Abernathey <ryan.abernathey@gmail.com>
Co-authored-by: Ryan Abernathey <ryan.abernathey@gmail.com>
Co-authored-by: Ryan Abernathey <ryan.abernathey@gmail.com>
@jbusecke
Copy link
Contributor Author

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.

@rabernat
Copy link
Contributor

rabernat commented Nov 11, 2020

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

@jbusecke
Copy link
Contributor Author

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

@tomchor
Copy link

tomchor commented Jan 5, 2021

@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

@jbusecke
Copy link
Contributor Author

jbusecke commented Jan 5, 2021

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!

@tomchor
Copy link

tomchor commented Jan 5, 2021

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.

@github-actions
Copy link

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!

@github-actions github-actions bot added the Stale label Jun 11, 2021
@github-actions
Copy link

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

@github-actions github-actions bot closed this Jun 18, 2021
@jbusecke jbusecke reopened this Jun 24, 2021
@jbusecke jbusecke added In Progress Somebody is working on fixing this issue and removed Stale labels Jun 24, 2021
@rcaneill
Copy link
Contributor

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

@jbusecke
Copy link
Contributor Author

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?).

@rcaneill
Copy link
Contributor

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)

@jbusecke
Copy link
Contributor Author

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

@jbusecke
Copy link
Contributor Author

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

@jbusecke
Copy link
Contributor Author

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

@rcaneill
Copy link
Contributor

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

@jbusecke
Copy link
Contributor Author

Perfect!

@rcaneill
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
In Progress Somebody is working on fixing this issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Quick Start Guide for docs main page[DOC]
4 participants