`static_column_depth` interface and numerical bottom height in `AbstractGridFittedBottom` #3841

simone-silvestri · 2024-10-09T08:30:10Z

This PR introduces an interface for static_column_depth that returns the height of the water column (not including the free surface).
This is simply grid.Lz in regular grids.

For AbstractGridFittedBottom immersed grids, the column height is readily read from the immersed boundary, provided that the immersed boundary represents the numerical bottom rather than the physical bottom.

Therefore, this PR changes the constructor of the ImmersedBoundaryGrid to store the z-coordinate of the last immersed cell. In this way, the bottom height is also uniquely defined for GridFittedBottom, removing the necessity of having aCenterImmersedCondition and a InterfaceImmersedCondition.

An immediate application of this interface is in the SplitExplicitFreeSurface where the bottom_height was previously stored.

simone-silvestri · 2024-10-09T12:48:28Z

Correction: InterfaceImmersedCondition and CenterImmersedCondition might still useful during construction because they inform where the bottom height will be.

glwagner · 2024-10-09T15:32:41Z

Is column_height a good name?

There's a naming issue. We had previously referred to "z" as "height". Usually this doesn't matter but when it does, eg in realistic contexts and using nonlinear equation of state, then we are trying to be consistent with the literature where "z" is the "geopotential height".

We can change the user-facing naming and notation for sure but it has to be done carefully and with consensus. I think this PR makes some big changes to notation and language that could have wide-ranging impacts on how we communicate about numerical models.

glwagner · 2024-10-09T15:33:19Z

src/ImmersedBoundaries/grid_fitted_bottom.jl


 struct GridFittedBottom{H, I} <: AbstractGridFittedBottom{H}
-    bottom_height :: H
+    z_bottom :: H


why this change?

I though we want to refer to z as the coordinate and height as a positive value.
At least, this is the direction we are taking in ClimaOcean where we are distinguishing very carefully between z and h or height.
In this case, this is technically a coordinate, not a height, for example we have now

Oceananigans.jl/src/ImmersedBoundaries/grid_fitted_bottom.jl

Line 109 in 989108a

@inline z_bottom(i, j, ibg::GFBIBG) = @inbounds ibg.immersed_boundary.bottom_height[i, j, 1]

which conflates the meaning of z with height. I thought we had decided this over at ClimaOcean and I wanted to update this code to reflect the decision we had made.

I though we want to refer to z as the coordinate and height as a positive value.

Usually "z" is called "geopotential height". In the way it's usually used, height can be positive or negative; the point is that it increases as we go upwards away from the center of the Earth (as opposed to "depth" which increases downwards). What would you call "z" instead? Altitude?

this is technically a coordinate, not a height

Another name for z is "height coordinate".

As someone who hasn't used immersed boundaries that much, I always find it a bit confusing that it's called bottom height because it makes me think I'm meant to put in a positive "height" from the bottom of the domain, but I would know that "z" is meant to be from the top

glwagner · 2024-11-06T18:03:49Z

src/ImmersedBoundaries/ImmersedBoundaries.jl

@@ -92,10 +95,6 @@ with_halo(halo, ibg::ImmersedBoundaryGrid) =
 inflate_halo_size_one_dimension(req_H, old_H, _, ::IBG)            = max(req_H + 1, old_H)
 inflate_halo_size_one_dimension(req_H, old_H, ::Type{Flat}, ::IBG) = 0

-# Defining the bottom
-@inline z_bottom(i, j, grid) = znode(i, j, 1, grid, c, c, f)
-@inline z_bottom(i, j, ibg::IBG) = error("The function `bottom` has not been defined for $(summary(ibg))!")


isn't this still useful? Why delete it?

I have moved it in the TurbulenceClosures module where also z_top and height_above_bottom are defined. I think it's cleaner to have all these functions in the same place. If I have to guess why they were not is because the ImmersedBoundaries module was defined after the TurbulenceClosures module. Since this is not the case anymore, this is a bit of a cleanup.

Ok so its unrelated to the static column depth? I feel z_bottom belongs in the Grids module. What do you think?

Consider that it has to be extended for different immersed boundary implementation so it depends on partial cell bottom, etc.

The original implementation may not have been optimal, but since we are moving things around here, I think we should try to improve on the original organization.

I am ok with moving all these functions in the Grids module, shall we do it in this PR?

glwagner · 2024-11-06T18:05:06Z

src/ImmersedBoundaries/grid_fitted_bottom.jl


-Adapt.adapt_structure(to, ib::GridFittedBottom) = GridFittedBottom(adapt(to, ib.bottom_height),
-                                                                             ib.immersed_condition)
+@inline static_column_depthᶠᶜᵃ(i, j, ibg::XFlatAGFIBG) = static_column_depthᶜᶜᵃ(i, j, ibg)


when this happens it could suggest that this should be more like node with

static_column_depth(i, j, grid, lx, ly)

glwagner · 2024-11-06T18:06:53Z

I'm confused about this PR. There are all sorts of changes in different places. There is even a change to the Makie extension. What am I reviewing?

simone-silvestri · 2024-11-07T10:01:00Z

I have merged in #3880 because I wanted to see if the GPU tests pass. I am planning to merge this PR after that one. The rest is all from this PR. The OceananigansMakieExt change is a typo fix, but I see you have added a PR for it.

glwagner · 2024-11-07T12:55:13Z

src/TurbulenceClosures/TurbulenceClosures.jl

-@inline depthᶜᶜᶜ(i, j, k, grid)    = clip(z_top(i, j, grid) - znode(i, j, k, grid, c, c, c))
-@inline total_depthᶜᶜᵃ(i, j, grid) = clip(z_top(i, j, grid) - z_bottom(i, j, grid))
+@inline depthᶜᶜᶠ(i, j, k, grid) = clip(z_top(i, j, grid) - znode(i, j, k, grid, c, c, f))
+@inline depthᶜᶜᶜ(i, j, k, grid) = clip(z_top(i, j, grid) - znode(i, j, k, grid, c, c, c))


is this used?

seems to be used in CATKE here for example

Oceananigans.jl/src/TurbulenceClosures/turbulence_closure_implementations/TKEBasedVerticalDiffusivities/catke_mixing_length.jl

Line 62 in 6c40d7e

d_up = Cˢ * depthᶜᶜᶠ(i, j, k, grid)

ah yes, this is local depth vs the column depth

glwagner · 2024-11-07T12:59:51Z

src/ImmersedBoundaries/partial_cell_bottom.jl

+        # If the size of the bottom cell is less than ϵ Δz, use 
+        # a simple `GridFittedBottom` approach where the bottom 
+        # height is the top interface of cell k.


This is a different numerical method than what we implemented, isn't it? Previously we would "open up" small cells. This seems to "close" them.

Which is a fine proposal but I think you should change the title of the PR. Imagine us looking for what is causing this kind of different in the future, combing through PRs. It's pretty hidden in this one

Could end up being the most important change in this PR

Hmmm, I thought there was supposed to be no difference.
Here, the bottom_height is set to the actual bottom height if it is smaller than z⁻ + ϵ Δz; otherwise, the cell is closed.
So, the only difference with the main branch will be for bottom heights that are in between z⁻ + ϵ Δz and z⁺ (where z⁻ and z⁺ are the interfaces of the bottom cell).
I will check the difference in the internal_tide example.

But I thought before we would keep the cell open (rather than closing it).

Actually I think that MITgcm uses some criteria to decide which one to do --- either snapping up or down depending on which change is least.

Ah, ok, I understand; open till ϵ Δz instead of snapping up. Yeah, this is definitely different.

Actually I think we used to use that criteria too. It's something like

partial_Δz = z⁺ - zb too_thin = partial_Δz < ϵ * Δz snap_up = partial_Δz < ϵ * Δz / 2 snapped_zb = ifelse(snap_up, z⁺, z⁻ + ϵ * Δz) numerical_zb = ifelse(too_thin, snapped_zb, zb)

glwagner · 2024-11-07T13:05:02Z

src/ImmersedBoundaries/partial_cell_bottom.jl

+        # If the size of the bottom cell is less than ϵ Δz, use 
+        # a simple `GridFittedBottom` approach where the bottom 
+        # height is the top interface of cell k.
+        capped_zb   = ifelse(zb < z⁻ + Δz * (1 - ϵ), zb, z⁺)


Suggested change

capped_zb = ifelse(zb < z⁻ + Δz * (1 - ϵ), zb, z⁺)

partial_Δz = z⁺ - zb

too_thin = partial_Δz < ϵ * Δz

adjusted_zb = ifelse(too_thin, z⁺, zb) # adjust bottom height to eliminate thin cells

this just makes it easier to understand. I was having trouble with the math. Also I think it's convoluted to evaluate the criterion by using the interface below and then adding Δz to both sides, isn't it?

right, I have modified it

src/ImmersedBoundaries/partial_cell_bottom.jl

glwagner · 2024-11-07T13:06:18Z

can you run and show is the results from the internal tide example just to be sure, because this PR changes the partial cell algorithm

simone-silvestri · 2024-11-07T15:43:39Z

This is the difference in the internal tide example. I hadn't appreciated this difference. Maybe I can revert the implementation so that this PR does not really change the behavior, and we can look more in-depth at what MITgcm does in another PR.
I also think we need some tests for the Partial cells, so that would be a good excuse to also look at the formulation.

internal_tide.mp4

simone-silvestri · 2024-11-07T16:30:13Z

In the new commit, I reverted to the previous implementation, so now there is no difference

internal_tide.mp4

simone-silvestri added 4 commits October 9, 2024 10:09

new bottom height

4044e16

now it should work

b83a6ae

comment

5ebaa84

comment

f9ad227

simone-silvestri changed the title ~~Bottom height for ImmersedBoundaries~~ bottom_height interface Oct 9, 2024

simone-silvestri added 3 commits October 9, 2024 10:54

remove circular dependency for now

cbdb713

some bugfixes

7f08768

change name to column_height

4b7926f

simone-silvestri changed the title ~~bottom_height interface~~ column_height interface Oct 9, 2024

simone-silvestri added 16 commits October 9, 2024 11:30

correct column height

6b7b27c

whoops

fdee366

another correction

d1c145c

some more changes

dbb411a

another correction

3ec05df

couple of more bugfixes

4e4d40b

more bugfixes

a6362ee

this should make it work

3bce6fc

unify the formulations

0fa1a67

correct implementation

135cac3

correct implementation

156dada

correct partial cell bottom

04e7170

use center immersed condition for grid fitted boundary

9258572

use the *correct* center node

9df333a

no h for z-values!

053b26f

simplify partial cells

61b7e7f

simone-silvestri added 2 commits October 9, 2024 14:55

make sure we don't go out of bounds

1098c58

back to immersed condition

9e35af6

glwagner reviewed Oct 9, 2024

View reviewed changes

simone-silvestri added 11 commits November 6, 2024 10:56

why are we using 8 threads?

9c3c6cd

memory requirements are not this huge

3b28ecb

update enzyme

00f87a4

speed up the precompilation a bit, to revert later

7126c7c

might this be the culprit?

733ab2b

revert to 8 tasks to precompile

2dbf1a0

final version?

a4b129a

return to previous state of affairs

29f7d69

Merge branch 'ss/fix-gpu-tests' into ss/new-bottom-height

56ded6a

bugfix

3154a34

cuda runtime version

3803ff3

glwagner reviewed Nov 6, 2024

View reviewed changes

glwagner reviewed Nov 7, 2024

View reviewed changes

src/ImmersedBoundaries/partial_cell_bottom.jl Outdated Show resolved Hide resolved

simone-silvestri changed the title ~~static_column_depth interface~~ static_column_depth interface and numerical bottom height in AbstractGridFittedBottom Nov 7, 2024

revert changes

2813769

glwagner approved these changes Nov 7, 2024

View reviewed changes

simone-silvestri added 3 commits November 8, 2024 08:58

Merge branch 'main' into ss/new-bottom-height

4b7d9ed

Merge branch 'main' into ss/new-bottom-height

3ab5bf3

revert

7741b9e

simone-silvestri merged commit ec4ac22 into main Nov 9, 2024
46 checks passed

simone-silvestri deleted the ss/new-bottom-height branch November 9, 2024 11:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`static_column_depth` interface and numerical bottom height in `AbstractGridFittedBottom` #3841

`static_column_depth` interface and numerical bottom height in `AbstractGridFittedBottom` #3841

simone-silvestri commented Oct 9, 2024 •

edited

Loading

simone-silvestri commented Oct 9, 2024

glwagner commented Oct 9, 2024 •

edited

Loading

glwagner Oct 9, 2024

simone-silvestri Oct 9, 2024

glwagner Oct 9, 2024 •

edited

Loading

glwagner Oct 9, 2024

jagoosw Oct 10, 2024

glwagner Nov 6, 2024

simone-silvestri Nov 7, 2024 •

edited

Loading

glwagner Nov 7, 2024

glwagner Nov 7, 2024

simone-silvestri Nov 7, 2024

glwagner Nov 6, 2024

glwagner commented Nov 6, 2024

simone-silvestri commented Nov 7, 2024

glwagner Nov 7, 2024

simone-silvestri Nov 7, 2024

glwagner Nov 7, 2024

glwagner Nov 7, 2024

glwagner Nov 7, 2024

glwagner Nov 7, 2024

simone-silvestri Nov 7, 2024 •

edited

Loading

glwagner Nov 7, 2024

simone-silvestri Nov 7, 2024 •

edited

Loading

glwagner Nov 7, 2024

glwagner Nov 7, 2024 •

edited

Loading

glwagner Nov 7, 2024

simone-silvestri Nov 7, 2024

glwagner commented Nov 7, 2024

simone-silvestri commented Nov 7, 2024

simone-silvestri commented Nov 7, 2024

-        capped_zb   = ifelse(zb < z⁻ + Δz * (1 - ϵ), zb, z⁺)
+        partial_Δz = z⁺ - zb
+        too_thin = partial_Δz < ϵ * Δz
+        adjusted_zb  = ifelse(too_thin, z⁺, zb) # adjust bottom height to eliminate thin cells

static_column_depth interface and numerical bottom height in AbstractGridFittedBottom #3841

static_column_depth interface and numerical bottom height in AbstractGridFittedBottom #3841

Conversation

simone-silvestri commented Oct 9, 2024 • edited Loading

simone-silvestri commented Oct 9, 2024

glwagner commented Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glwagner Oct 9, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simone-silvestri Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glwagner commented Nov 6, 2024

simone-silvestri commented Nov 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simone-silvestri Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simone-silvestri Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glwagner Nov 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

glwagner commented Nov 7, 2024

simone-silvestri commented Nov 7, 2024

simone-silvestri commented Nov 7, 2024

`static_column_depth` interface and numerical bottom height in `AbstractGridFittedBottom` #3841

`static_column_depth` interface and numerical bottom height in `AbstractGridFittedBottom` #3841

simone-silvestri commented Oct 9, 2024 •

edited

Loading

glwagner commented Oct 9, 2024 •

edited

Loading

glwagner Oct 9, 2024 •

edited

Loading

simone-silvestri Nov 7, 2024 •

edited

Loading

simone-silvestri Nov 7, 2024 •

edited

Loading

simone-silvestri Nov 7, 2024 •

edited

Loading

glwagner Nov 7, 2024 •

edited

Loading