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

The start of documentation 2.0 #3673

Merged
merged 116 commits into from
Aug 10, 2024
Merged

The start of documentation 2.0 #3673

merged 116 commits into from
Aug 10, 2024

Conversation

glwagner
Copy link
Member

@glwagner glwagner commented Jul 30, 2024

This PR reorders the documentation and will add a Grids and Fields page to the top-level of the docs. It will also clean up minor outstanding issues.

Longer description of our intent: #3672

Closes #3649

@glwagner
Copy link
Member Author

@navidcy I have also enabled push previews for this PR but we can disable before merging.

@glwagner glwagner marked this pull request as draft July 30, 2024 18:55
@navidcy navidcy added the documentation 📜 The sacred scrolls label Jul 30, 2024
@glwagner
Copy link
Member Author

We'll be looking here for previews: https://clima.github.io/OceananigansDocumentation/previews/PR3673

@glwagner
Copy link
Member Author

@navidcy I think it would be cool to show a visualization of the grids in the "Grids tutorial"

@francispoulin
Copy link
Collaborator

We'll be looking here for previews: https://clima.github.io/OceananigansDocumentation/previews/PR3673

I tried this link but received a 404 error. Does it work for others?

@glwagner
Copy link
Member Author

We'll be looking here for previews: https://clima.github.io/OceananigansDocumentation/previews/PR3673

I tried this link but received a 404 error. Does it work for others?

It errors for me too! Nothing will come up until the docs build passes, looks like it is still failing.

@glwagner
Copy link
Member Author

glwagner commented Aug 2, 2024

I think we should improve the docstring for Distributed:

"""
Distributed(child_architecture = CPU();
communicator = MPI.COMM_WORLD,
devices = nothing,
synchronized_communication = false,
partition = Partition(MPI.Comm_size(communicator)))
Return a distributed architecture that uses MPI for communications.
Positional arguments
====================
- `child_architecture`: Specifies whether the computation is performed on CPUs or GPUs.
Default: `CPU()`.
Keyword arguments
=================
- `synchronized_communication`: If `true`, always use synchronized communication through ranks.
Default: `false`.
- `partition`: A [`Partition`](@ref) specifying the total processors in the `x`, `y`, and `z` direction.
Note that support for distributed `z` direction is limited; we strongly suggest
using partition with `z = 1` kwarg.
- `devices`: `GPU` device linked to local rank. The GPU will be assigned based on the
local node rank as such `devices[node_rank]`. Make sure to run `--ntasks-per-node` <= `--gres=gpu`.
If `nothing`, the devices will be assigned automatically based on the available resources
- `communicator`: the MPI communicator, `MPI.COMM_WORLD`. This keyword argument should not be tampered with
if not for testing or developing. Change at your own risk!
"""

A few comments:

  • Can we explain "synchronized communication" better? This is confusing --- I think it actually has to do with the algorithm a model uses (very far away from building the architecture). We have to explain that this concept is irrelevant unless we are using a model that has an asychronous algorithm. Or otherwise more specifically explain what this means.
  • The entry for devices is missing a period a the end.

In general more explanation of the keyword arguments that is as local to the concept of grids as possible. The docstring makes vague references to "support for partitioning". But this refers to models, not grids. It doesn't really make sense in this context.

Also we need examples.

docs/src/grids.md Outdated Show resolved Hide resolved
@navidcy
Copy link
Collaborator

navidcy commented Aug 10, 2024

The distributed doctests in grids.md don't run, somehow the environment fails to install?

@navidcy
Copy link
Collaborator

navidcy commented Aug 10, 2024

@glwagner I removed the "Distributed Grids" part from the docs. I suggest we merge this if Docs built and open a new PR with the distributed grids part of the tutorial?

@navidcy navidcy merged commit 866a33b into main Aug 10, 2024
46 checks passed
@glwagner glwagner deleted the glw/docs2 branch August 12, 2024 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation 📜 The sacred scrolls
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Outdated docs on background diffusivity/viscosity
4 participants