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

Fill metrics for halo regions for OrthogonalSphericalShellGrid #3239

Merged
merged 34 commits into from
Sep 19, 2023

Conversation

navidcy
Copy link
Collaborator

@navidcy navidcy commented Aug 28, 2023

This PR fills the metrics for the halo regions for a OrthogonalSphericalShellGrid.

Also adds a validation example for a splash on an OrthogonalSphericalShellGrid, i.e., a HydrostaticFreeSurface model starting from rest + its free surface lifted up in the form of a Gaussian.

Partially resolves #3198.

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.

beautiful


for j in ny-Hy:-1:Hy+1
for i in Hx:-1:1
parent(metric)[i, j] = parent(metric)[i+1, j]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does the metric not reflect across the boundaries?

metric[1-i, j] = metric[i, j]

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't understand exactly what you mean here @simone-silvestri

Copy link
Collaborator

Choose a reason for hiding this comment

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

what is the rule for the metrics in the halo? I would expect them to reflect across the boundary following this rule:

metric[1-i, j] = metric[i, j]

but here you seem to be implying that all metrics in the boundary are equal to the last metric in the interior

parent(metric)[i, j] = parent(metric)[i+1, j]

what is the correct rule here? I think we need halo metrics to be filled with a fill halo to be sure because neither of the options is always correct.

Copy link
Collaborator Author

@navidcy navidcy Aug 28, 2023

Choose a reason for hiding this comment

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

I don’t understand what you mean by “reflect”. In general it is ambiguous how one can fill the metrics and thus I chose to fill them according to their nearest interior value. For FullyConnected topologies, of course, the halo regions correspond to the metrics of the interior neighboring region.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It should be like that also for Periodic which takes from the other side of the face. In that case (and FullyConnected) the metrics are not so ambiguous. The only instance where metrics in the halos can be considered equal to the interior ones (or ambiguous) is for solid walls and value/gradient BC (where we only need 1 halo). If we don't fill them correctly we might have distortions across the boundary which can crash the simulation/kick off numerical instabilities that grow and pollute the solution (see #3091 where metrics where not correctly considered in reconstructions)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, I found that also and I tried to replicate that functionality. I think I did it correctly.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I couldn't use that lower_exterior_Δcoordᶠ method; unfortunately I don't remember now why... it was work done over a long flight which hints that I may not have come to that conclusion at my full brain capacity. :(

I'll revisit and see if I can use that utility here.

Copy link
Collaborator

@simone-silvestri simone-silvestri Sep 12, 2023

Choose a reason for hiding this comment

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

I wouldn't stress too much about this at the moment. It is kind of a low priority because, in every practical application of the OSSG, the metrics should be filled with a fill_halo_regions! (not only forConnected but also for Periodic topologies. This might be akin to "extending" in some cases). I agree with extending for Bounded halo metrics but they matter only for Value and Gradient boundary conditions

Copy link
Member

Choose a reason for hiding this comment

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

Aren't we planning to replace lat-lon grid with OSSG? What happened to that plan?

Copy link
Member

Choose a reason for hiding this comment

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

I personally would like to move forward with a fully function OSSG so I can use it for regional simulations (eg in the arctic) that are not restricted to borders that exactly align with geographic coordinates. It will also be nice to rotate domains to follow coastlines, etc

@navidcy
Copy link
Collaborator Author

navidcy commented Sep 7, 2023

@simone-silvestri can you have a look now?

@navidcy
Copy link
Collaborator Author

navidcy commented Sep 8, 2023

@simone-silvestri I plot here how the metric look for a series of overlapping conformal cubed sphere panels constructed with this PR.

using Oceananigans
using Oceananigans.Grids
using Oceananigans.MultiRegion: getregion
using GLMakie

Nx, Ny = 8, 8
H = 4

range_with_offset(N, H, offset::Int) = (-H + 1 + offset*N):(N + H + offset*N)

function plot_series_ossg(grid, filename)
    j_index = 2

    fig = Figure(resolution=(2200, 600), fontsize=30)
    ax = Axis(fig[1, 1])
    lines!(ax, range_with_offset(Nx, H, 0), parent(grid.Δxᶜᶜᵃ)[:, j_index+H], linewidth=4, color = (:green, 0.9), label="an ossg")
    lines!(ax, range_with_offset(Nx, H, 1), parent(grid.Δxᶜᶜᵃ)[:, j_index+H], linewidth=8, color = (:red, 0.5), label="another ossg")
    lines!(ax, range_with_offset(Nx, H, 2), parent(grid.Δxᶜᶜᵃ)[:, j_index+H], linewidth=4, color = (:blue, 0.5), label="another ossg")
    vlines!(ax, [1, Nx+1, 2Nx+1, 3Nx+1], linewidth=8, color=(:black, 0.3))
    fig[1, 2] = Legend(fig, ax, framevisible = false)
    save(filename, fig)

    return fig
end

grid = conformal_cubed_sphere_panel(size=(Nx, Ny, 1), z=(-1, 0), halo=(H, H, 1))
plot_series_ossg(grid, "metric_test_ossg_bounded.png")

grid = conformal_cubed_sphere_panel(size=(Nx, Ny, 1), z=(-1, 0), halo=(H, H, 1), topology=(Periodic, Periodic, Bounded))
plot_series_ossg(grid, "metric_test_ossg_periodic.png")

For Bounded topology:

metric_test_ossg_bounded

For Periodic topology:

metric_test_ossg_periodic

Isn't this what you were trying to imply they should look like?

navidcy and others added 3 commits September 11, 2023 20:07
Replaced OrthogonalSphericalShellGrid with conformal_cubed_sphere_panel (after
importing it) in validation/othogonal_spherical_shell_grid/splash.jl.
@glwagner
Copy link
Member

This looks great and it's consistent with what generate_coordinate does

function generate_coordinate(FT, topo::AT, N, H, coord, arch)

@navidcy
Copy link
Collaborator Author

navidcy commented Sep 18, 2023

@glwagner with 8d8678a I changed to using normal indexing (not parent-array indices). The results of the script above remain unchanged!

Is this OK to merge now?

@navidcy navidcy merged commit a1031f9 into main Sep 19, 2023
47 checks passed
@navidcy navidcy deleted the ncc/ossg-halo-metrics branch September 19, 2023 19:24
navidcy added a commit that referenced this pull request Sep 20, 2023
* fill halo points for metrics

* add splash validation on ossg grid

* Update src/Grids/orthogonal_spherical_shell_grid.jl

Co-authored-by: Gregory L. Wagner <[email protected]>

* Update src/Grids/orthogonal_spherical_shell_grid.jl

Co-authored-by: Gregory L. Wagner <[email protected]>

* simplify code

* add comment for the way halo-corner metrics are filled

* simplify code

* only fill halo regions

* test only at interior points

* remove stray spaces

* code alignment

* remove extra empty line

* code alignment + fix some comments

* fill halo metrics appropriately based on coord topo

* better docs for rotation kwarg + one more example

* enhance comment on distance metrics + code alignment

* add rotation in conformal_mapping field

* add docstrings for fill_metric_halo_regions methods

* reorder

* OSSG --> conformal_cubed_sphere_panel

Replaced OrthogonalSphericalShellGrid with conformal_cubed_sphere_panel (after
importing it) in validation/othogonal_spherical_shell_grid/splash.jl.

* use normal indices for metric halo fillings, not parent ones

* cleaner fill_metric_halo_regions_x/y/corners

* add missing space

* fix typo + cleanup

* delete

* less diffusion

---------

Co-authored-by: Gregory L. Wagner <[email protected]>
Co-authored-by: Sid <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OrthogonalSphericalShellGrid metrics and coordinates are missing values in halo points
4 participants