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.

Sign up for GitHub

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

Jump to bottom

Update several Docs sections + docstrings #2639

Merged
merged 38 commits into from
Jul 15, 2022
Merged

Update several Docs sections + docstrings #2639

merged 38 commits into from
Jul 15, 2022

Conversation

elise-palethorpe
Copy link
Collaborator

@elise-palethorpe elise-palethorpe commented Jul 4, 2022 •
edited by navidcy
Loading

This PR updates several sections of the Docs to add clarity. In particular:

  • Physics/Boussinesq approximation
  • Numerical Implementation/Elliptic Solvers/Implicit free surface pressure operator
  • Numerical Implementation/Pressure decomposition
  • Numerical Implementation/Time-stepping
  • HydrostaticFreeSurfaceModel docstrings

Also, closes #2653 and closes #2656.

@navidcy navidcy added the documentation 📜 The sacred scrolls label Jul 4, 2022
@elise-palethorpe @navidcy
Update docs/src/numerical_implementation/poisson_solvers.md
ed81e41 
Co-authored-by: Navid C. Constantinou <navidcy@users.noreply.github.com>
@navidcy
Copy link
Collaborator

navidcy commented Jul 4, 2022 •
edited
Loading

A preview of the docs will be available at

https://clima.github.io/OceananigansDocumentation/previews/PR2639

(after the docs are build... it takes ~2hrs)

francispoulin
francispoulin reviewed Jul 4, 2022
View reviewed changes
Copy link
Collaborator

@francispoulin francispoulin left a comment

Choose a reason for hiding this comment

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

The change looks good to me but when I click on the two links I get 404 file not founds. Any idea why that might be?

@navidcy
Copy link
Collaborator

navidcy commented Jul 4, 2022

@elise-palethorpe what do you think about 1b69908?

@navidcy
Copy link
Collaborator

navidcy commented Jul 5, 2022 •
edited
Loading

I think eq 9 in https://clima.github.io/OceananigansDocumentation/previews/PR2639/numerical_implementation/poisson_solvers/ is correct; am I right?

if so, I seem to have trouble deriving eq 10…

@navidcy
Copy link
Collaborator

navidcy commented Jul 5, 2022

I think eq 9 in https://clima.github.io/OceananigansDocumentation/previews/PR2639/numerical_implementation/poisson_solvers/ is correct; am I right?

if so, I seem to have trouble deriving eq 10…

OK, I see the derivation in pages 7-8 of https://www.overleaf.com/project/6042a885d8327860fae7cc5e.
I think we need to include a bit more steps from this derivation here...

@francispoulin
Copy link
Collaborator

I think eq 9 in https://clima.github.io/OceananigansDocumentation/previews/PR2639/numerical_implementation/poisson_solvers/ is correct; am I right?
if so, I seem to have trouble deriving eq 10…

OK, I see the derivation in pages 7-8 of https://www.overleaf.com/project/6042a885d8327860fae7cc5e. I think we need to include a bit more steps from this derivation here...

Ageed. It seems that we integrate the continuity equation from the bottom, z=-H, to the surface, z=0, exchange the order of integration and differentiation, and then substitute in the kinetmatic boundary conditions at the top and bottom. But maybe even more detail would be helpful?

@navidcy navidcy changed the title Update implicit free surface pressure operator docs Update implicit free surface pressure operator docs + some other fixes in the Docs Jul 13, 2022
@navidcy
Copy link
Collaborator

navidcy commented Jul 13, 2022

@elise-palethorpe is the pressure decomposition section done in 2423801 cleaner?

francispoulin
francispoulin reviewed Jul 14, 2022
View reviewed changes
docs/src/numerical_implementation/elliptic_solvers.md Outdated
and ``M`` is some surface volume flux (e.g., terms such as precipitation, evaporation and runoff); currently the
model takes ``M = 0``.

To form a linear system that can be solved implicitly we recast the continuity equation into a discrete
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is really integrating over the volume. Can we say this explicitly?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you elaborate a bit more?

But yes, these are the sort of things we'd like to clarify in the PR!

Copy link
Collaborator

@navidcy navidcy Jul 15, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I added a bunch of clarifications. Hopefully I've addressed your concern @francispoulin?

@navidcy
Copy link
Collaborator

navidcy commented Jul 15, 2022

@elise-palethorpe if you are happy with this PR merge when all tests pass ;)

@navidcy navidcy changed the title Update implicit free surface pressure operator docs + some other fixes in the Docs Update several Docs sections + docstrings Jul 15, 2022
@navidcy navidcy merged commit 2edd2b1 into main Jul 15, 2022
@navidcy navidcy deleted the ep/docs-enhancement branch July 15, 2022 15:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@francispoulin francispoulin francispoulin approved these changes

@navidcy navidcy navidcy approved these changes

@simone-silvestri simone-silvestri Awaiting requested review from simone-silvestri

@glwagner glwagner Awaiting requested review from glwagner

Assignees
No one assigned
Labels
documentation 📜 The sacred scrolls
Projects
None yet
Milestone
No milestone
3 participants
@elise-palethorpe @navidcy @francispoulin