Skip to content

Move storage space to from MutableVerticalDiscretization to ZStarCoordinate #4686

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
Open
wants to merge 21 commits into
base: main
Choose a base branch
from
Open

Move storage space to from MutableVerticalDiscretization to ZStarCoordinate #4686

wants to merge 21 commits into from
+78 −65

Conversation

simone-silvestri
Copy link
Collaborator

and while we are at it, also refactor ZStar to ZStarCoordinate

glwagner
glwagner approved these changes Jul 30, 2025
View reviewed changes
Copy link
Member

@glwagner glwagner left a comment

Choose a reason for hiding this comment

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

Is ηⁿ needed to compute metrics too? Basically I think the only information that should be in the grid is what is required to compute the metrics. Anything to do with specific evolution rules is a model concept?

@simone-silvestri
Copy link
Collaborator Author

simone-silvestri commented Jul 30, 2025 •
edited
Loading

Yeah, ηⁿ is the surface of the domain, so it is needed for znode to compute the correct z level. It might be considered somewhat a zstar specific quantity, but I guess any vertical coordinate would require the information of the extent of the domain, including the surface height.

@glwagner
Copy link
Member

Yeah, ηⁿ is the surface of the domain, so it is needed for znode to compute the correct z level. It might be considered somewhat a zstar specific quantity, but I guess any vertical coordinate would require the information of the extent of the domain, including the surface height.

Okay, so the main improvement would be better notation if we have it

@glwagner
Copy link
Member

What about the time derivative of the stretching metric? This seems unnecessary for prescribed evolution?

navidcy
navidcy reviewed Jul 30, 2025
View reviewed changes
navidcy
navidcy reviewed Jul 30, 2025
View reviewed changes
@simone-silvestri
Copy link
Collaborator Author

simone-silvestri commented Jul 30, 2025 •
edited
Loading

What about the time derivative of the stretching metric? This seems unnecessary for prescribed evolution?

I think that is required for any type of mutable grid, but I guess that can also be moved...

Might be annoying to rewrite this operator though

Oceananigans.jl/src/Advection/vector_invariant_self_upwinding.jl

Lines 8 to 12 in fb96739

# For moving grids, we include the time-derivative of the grid scaling in the divergence flux.
# If the grid is stationary, `Az_Δr_∂t_σ` evaluates to zero.
@inline δx_U_plus_∂t_σ(i, j, k, grid, u, v) = δxᶜᶜᶜ(i, j, k, grid, Ax_qᶠᶜᶜ, u) + Az_Δr_∂t_σ(i, j, k, grid)
@inline δy_V_plus_∂t_σ(i, j, k, grid, u, v) = δyᶜᶜᶜ(i, j, k, grid, Ay_qᶜᶠᶜ, v) + Az_Δr_∂t_σ(i, j, k, grid)

@navidcy navidcy self-requested a review July 30, 2025 20:34
@navidcy
Copy link
Member

navidcy commented Jul 30, 2025 •
edited
Loading

Suggest bump patch release with this (or straight after) since it breaks this:

https://github.com/CliMA/ClimaOcean.jl/blob/8479089e933f1c8b29b9aaaac11e51437fbd9c64/src/OceanSimulations/ocean_simulation.jl#L80

@navidcy navidcy mentioned this pull request Jul 30, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@navidcy navidcy navidcy approved these changes

@glwagner glwagner glwagner approved these changes

Assignees
No one assigned
Labels
grids 🗺️
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@simone-silvestri @glwagner @navidcy