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 halos of conformal cubed sphere grid coordinates and metrics in a single pass #3488

Merged
merged 107 commits into from
Apr 4, 2024

Conversation

siddharthabishnu
Copy link
Contributor

@siddharthabishnu siddharthabishnu commented Mar 1, 2024

This PR fills the halos of the grid metrics of the conformal cubed sphere panels at face-face locations using a dedicated function instead of hard-coded values, similar to the existing approach for center-center, face-center, and center-face locations.

cc @navidcy

This commit integrates some key updates from PR #3306, specifically commit
941260f on branch ncc-glw/cubed-sphere-dynamics. We have replaced the following
scripts:
(a) src/Models/HydrostaticFreeSurfaceModels/
    update_hydrostatic_free_surface_model_state.jl;
(b) src/MultiRegion/multi_region_cubed_sphere_grid.jl;
with their modified versions from the PR. The main improvement involves
utilizing the fill_paired_halo_regions! function to fill the halos of grid
metrics on conformal cubed sphere panels at center-center, face-center, and
center-face locations.
@navidcy
Copy link
Collaborator

navidcy commented Mar 1, 2024

@siddharthabishnu, how do we know that now the metrics are filled correctly? you were comparing with a grid from MITgcm?

@navidcy navidcy changed the title Employ a function to fill halos of conformal cubed sphere grid metrics at face-face locations Fill halos of conformal cubed sphere grid metrics Mar 1, 2024
@siddharthabishnu
Copy link
Contributor Author

@siddharthabishnu, how do we know that now the metrics are filled correctly? you were comparing with a grid from MITgcm?

Yes. Consider the following sources for the grid metrics:

  1. the cs32 grid with one halo layer, used by:
    • Ali for the Rossby-Haurwitz test case in Oceananigans v0.82.0; and
    • yourself to check the interior coordinates and grid metrics of the Oceananigans cc32 grid;
  2. the cs32 grid with 4 halo layers created by @jm-c using MITgcm;
  3. the cc32 grid created by Oceananigans.

In the validation scripts for solid body rotation and the Rossby-Haurwitz wave within the ncc-glw/cubed-sphere-dynamics branch associated with PR #3306, I compared the metrics of grid (3) against both (1) and (2), and plotted their absolute and relative differences. With the latest modifications, these differences have been minimized though not entirely eliminated.

@navidcy
Copy link
Collaborator

navidcy commented Mar 5, 2024

@siddharthabishnu, how do we know that now the metrics are filled correctly? you were comparing with a grid from MITgcm?

Yes. Consider the following sources for the grid metrics:

  1. the cs32 grid with one halo layer, used by:

    • Ali for the Rossby-Haurwitz test case in Oceananigans v0.82.0; and
    • yourself to check the interior coordinates and grid metrics of the Oceananigans cc32 grid;
  2. the cs32 grid with 4 halo layers created by @jm-c using MITgcm;

  3. the cc32 grid created by Oceananigans.

In the validation scripts for solid body rotation and the Rossby-Haurwitz wave within the ncc-glw/cubed-sphere-dynamics branch associated with PR #3306, I compared the metrics of grid (3) against both (1) and (2), and plotted their absolute and relative differences. With the latest modifications, these differences have been minimized though not entirely eliminated.

OK, so the benchmark is the cs32 grid by MITgcm. Can we do the comparison in this PR? I'd like to see a test ideally because otherwise how do we assess that the changes we are suggesting here are correct. I can also do that, just give me a code snippet that loads the two grids?

@navidcy
Copy link
Collaborator

navidcy commented Mar 5, 2024

I cleaned the PR a bit. Didn't change anything.

@siddharthabishnu
Copy link
Contributor Author

siddharthabishnu commented Apr 4, 2024

@siddharthabishnu, CUDA.@allowscalar introduced by 3cdd470 is detrimental for performance. Like it induces O(10-100x) slowdown I think....
Is this a temporary solution?
cc @glwagner, @simone-silvestri

I think it is. We can try to see if this works on one GPU. If it does we can keep the allowscalar for the moment otherwise we can remove them. In the end all this will have to live in a kernel. Note that this will still not work on multiple GPUs as you cannot explicitly access one region from another one on a different GPU without switching to the device that holds the data.

@simone-silvestri and @navidcy, I totally agree. I only introduced CUDA.@allowscalar under the impression it was necessary for certain GPU tests to pass. However, now understanding that isn’t the case, I've removed it in commit 7f54c3c.

@@ -114,12 +114,10 @@ function fill_cubed_sphere_halo_regions!(field::CubedSphereField{<:Face, <:Face}
return nothing
end

fill_cubed_sphere_halo_regions!(fields::Tuple{CubedSphereField, CubedSphereField};
signed = true) = fill_cubed_sphere_halo_regions!(fields...; signed)
fill_halo_regions!(fields::Tuple{CubedSphereField, CubedSphereField}; signed = true) = fill_halo_regions!(fields...; signed)
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it clean/good idea to extend fill_halo_regions! for (::Tuple{Fields})? I guess we need something like that when dealing with vector-like quantities

cc @glwagner, @siddharthabishnu, @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.

there is already a method for filling the halos of tuples (and namedtuples) of fields. In that case, we launch kernels that fill all the fields together.
See

function fill_halo_regions!(maybe_nested_tuple::Union{NamedTuple, Tuple}, args...; kwargs...)

and
const MaybeTupledData = Union{OffsetArray, NTuple{<:Any, OffsetArray}}
"Fill halo regions in ``x``, ``y``, and ``z`` for a given field's data."
function fill_halo_regions!(c::MaybeTupledData, boundary_conditions, indices, loc, grid, args...; kwargs...)

Copy link
Collaborator

Choose a reason for hiding this comment

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

In case of a MultiRegionField (of which a CubedSphereField` is a subset) this behavior is bypassed

# This can be implemented once we have a buffer for field_tuples
@inline function tupled_fill_halo_regions!(full_fields, grid::MultiRegionGrids, args...; kwargs...)
for field in full_fields

@navidcy
Copy link
Collaborator

navidcy commented Apr 4, 2024

there are few outstanding questions still so I took back the approval -- sorry

@navidcy navidcy self-requested a review April 4, 2024 14:37
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.

4 participants