Fully connected cubed sphere #1522

ali-ramadhan · 2021-03-30T13:33:23Z

This PR adds a new submodule Oceananigans.CubedSpheres that implements new grid and field types for running HydrostaticFreeSurfaceModel with a fully connected cubed sphere (should eventually also be flexible enough to just use 1 < n < 6 faces but I'm just testing 6 faces for now).

This PR is still a work-in-progress and is quite messy with some pretty horrible code (please don't judge 😭). I'll be adding to it and refactoring it over the next few days.

Compile times have increased significantly with grid::ConformalCubedSphereGrid which has slowed down development but should be pretty close to being able to time step. EDIT: This might only be on Julia 1.6...

francispoulin · 2021-03-30T17:16:24Z

test/test_diagnostics.jl

-    cfl = DiffusiveCFL(FT(Δt))
+    Δt = FT(1.7)
+
+    Δx = model.grid.Δx


If you only use each of these grid spacings once, would it be better to just use the full name where they are used?

True. I think this change is part of #1497 but I'll revise those tests before merging.

francispoulin

I did look over all the changes and there certianly is a lot there. I don't feel like I understand all the different components enough to approve but I am curious to see the results and how they compare with predictions

ali-ramadhan · 2021-04-12T23:03:22Z

Coriolis seems fine now that grid metric halos are filled:

cubed_sphere_surface_gravity_waves_face1_with_coriolis.mp4

ali-ramadhan · 2021-04-14T02:25:16Z

Tests are on track to pass so I think this PR is finally ready to be reviewed/merged (with a regrettable amount of technical debt).

It's unfortunately too large to be effectively reviewed but I'm happy to make changes if needed (or go over the code on Zoom if anyone would like to do so).

christophernhill

@ali-ramadhan looks good for where we currently are. I left a few minor comments.

Is there somewhere to note that this is an intermediate "caveat emptor" that runs but is likely to change quite a bit in the future?

christophernhill · 2021-04-14T02:47:07Z

validation/cubed_sphere_surface_gravity_waves/surface_gravity_waves_on_face.jl

@@ -68,13 +68,7 @@ A  = 1e-5 * H  # Amplitude of the perturbation

 η′(λ, φ, z) = A * exp(- (λ - λ₀)^2 / Δλ^2) * exp(- (φ - φ₀)^2 / Δφ^2)


@ali-ramadhan I am getting unresolved method for η′(λ, φ, z) = .... (see below).
η′(λ, φ,) = .... works.

ERROR: LoadError: MethodError: no method matching (::var"#η′#2"{Float64,Int64,Int64,Int64,Int64})(::Float64, ::Float64) Closest candidates are: η′(::Any, ::Any, ::Any) at /Users/chrishill/projects/oceananigans/onan-cs-review/validation/cubed_sphere_surface_gravity_waves/cubed_sphere_surface_gravity_waves.jl:93 Stacktrace: [1] set!(::Oceananigans.Fields.ReducedField{Center,Center,Nothing,OffsetArrays.OffsetArray{Float64,3,Array{Float64,3}},ConformalCubedSphereFaceGrid{Float64,Periodic,Periodic,Bounded,OffsetArrays.OffsetArray{Float64,2,Array{Float64,2}},OffsetArrays.OffsetArray{Float64,1,StepRangeLen{Float64,Base.TwicePrecision{Float64},Base.TwicePrecision{Float64}}}},1,NamedTuple{(:x, :y, :z),Tuple{CoordinateBoundaryConditions{BoundaryCondition{Oceananigans.CubedSpheres.CubedSphereExchange,Oceananigans.CubedSpheres.CubedSphereExchangeInformation{Int64,Symbol}},BoundaryCondition{Oceananigans.CubedSpheres.CubedSphereExchange,Oceananigans.CubedSpheres.CubedSphereExchangeInformation{Int64,Symbol}}},CoordinateBoundaryConditions{BoundaryCondition{Oceananigans.CubedSpheres.CubedSphereExchange,Oceananigans.CubedSpheres.CubedSphereExchangeInformation{Int64,Symbol}},BoundaryCondition{Oceananigans.CubedSpheres.CubedSphereExchange,Oceananigans.CubedSpheres.CubedSphereExchangeInformation{Int64,Symbol}}},CoordinateBoundaryConditions{Nothing,Nothing}}}}, ::var"#η′#2"{Float64,Int64,Int64,Int64,Int64}) at /Users/chrishill/projects/oceananigans/onan-cs-review/src/CubedSpheres/cubed_sphere_set!.jl:54

Ah sorry about this! I forgot to change this since model.free_surface.η became a ReducedField. I'll update the validation experiments.

christophernhill · 2021-04-14T02:51:29Z

src/CubedSpheres/cubed_sphere_fields.jl

+
+    # This field needs BCs otherwise errors happen so I'll assume all faces have
+    # the same boundary conditions. A very bad assumption...
+    cubed_sphere_bcs = faces[1].boundary_conditions


Yeah haha, in hindsight I should have went with CubedSphereData and CubedSphereBoundaryConditions structs. But will do it next time we get a chance to refactor.

christophernhill · 2021-04-14T03:07:56Z

validation/cubed_sphere_rossby_haurwitz/cubed_sphere_rossby_haurwitz.jl

+    model = HydrostaticFreeSurfaceModel(
+              architecture = CPU(),
+                      grid = grid,
+        momentum_advection = VectorInvariant(),


@ali-ramadhan do we want to have this on here - since we know it is broken?

True. I'll revert back to momentum_advection = nothing for now until VectorInvariant() is correct for the cubed sphere.

christophernhill · 2021-04-14T03:10:41Z

validation/cubed_sphere_surface_gravity_waves/cubed_sphere_surface_gravity_waves.jl

+    model = HydrostaticFreeSurfaceModel(
+              architecture = CPU(),
+                      grid = grid,
+        momentum_advection = VectorInvariant(),


@ali-ramadhan do we want inertial terms in this setup? We could just do the linear problem for this example?

Ah I think I was just testing it but will change to momentum_advection = nothing for now, at least until VectorInvariant() is fixed for the cubed sphere.

ali-ramadhan · 2021-04-14T12:13:09Z

Thanks for taking a look @christophernhill!

Is there somewhere to note that this is an intermediate "caveat emptor" that runs but is likely to change quite a bit in the future?

Yes, we just have a warning that gets printed when you construct a cubed sphere grid saying that's it's an experimental feature:

Oceananigans.jl/src/CubedSpheres/conformal_cubed_sphere_grid.jl

Line 160 in 94dc9a2

@warn "ConformalCubedSphereGrid is experimental: use with caution!"

glwagner · 2021-04-14T14:11:45Z

src/CubedSpheres/conformal_cubed_sphere_grid.jl

+λnode(LX::Face, LY::Center, LZ, i, j, k, grid::ConformalCubedSphereFaceGrid) = grid.λᶠᶠᵃ[i, j]
+λnode(LX::Center, LY::Face, LZ, i, j, k, grid::ConformalCubedSphereFaceGrid) = grid.λᶠᶠᵃ[i, j]
+φnode(LX::Face, LY::Center, LZ, i, j, k, grid::ConformalCubedSphereFaceGrid) = grid.φᶠᶠᵃ[i, j]
+φnode(LX::Center, LY::Face, LZ, i, j, k, grid::ConformalCubedSphereFaceGrid) = grid.φᶠᶠᵃ[i, j]


Do these all need @inbounds?

glwagner · 2021-04-14T14:12:40Z

src/CubedSpheres/conformal_cubed_sphere_grid.jl

+# Not sure how to best represent these so will concatenate along dim 3 for now.
+
+λnodes(LX, LY, LZ, grid::ConformalCubedSphereGrid) = cat(Tuple(λnodes(LX, LY, LZ, grid_face) for grid_face in grid.faces)..., dims=3)
+φnodes(LX, LY, LZ, grid::ConformalCubedSphereGrid) = cat(Tuple(φnodes(LX, LY, LZ, grid_face) for grid_face in grid.faces)..., dims=3)


I think the point is probably to support broadcasting with CubedSphereData...

glwagner · 2021-04-14T14:14:52Z

src/Grids/conformal_cubed_sphere_face_grid.jl

-    offset_data = zeros(Nx + 1 + 2Hx, Ny + 1 + 2Hy)
-    offset_data[1+Hx:Nx+1+Hx, 1+Hy:Ny+1+Hy] .= data
-    return OffsetArray(offset_data, -Hx, -Hy)
+function offset_data_2d(data, loc, topo, N, H)


Hmm... this is really in xy (there are two other two dimensional configurations)

glwagner · 2021-04-14T14:15:55Z

src/Models/HydrostaticFreeSurfaceModels/hydrostatic_free_surface_ab2_step.jl

    # Launch velocity update kernels

    velocities_events = []

    for name in (:u, :v)
+        model.velocities isa PrescribedVelocityFields && break


This function should never be called when using PrescribedVelocityFields. This means there is a bug in the code in prescribed_velocity_fields.jl. This line shouldn't be here...

glwagner

I didn't review every file in the PR. Existing code looks largely untouched so I think we are fine. My only comment is that we definitely should not have any code that refers to PrescribedVelocityFields anywhere outside of prescribed_velocity_fields.jl. I'm ok to keep this here in the spirit of moving forward but the underlying issue should be addressed in the future.

ali-ramadhan · 2021-04-14T15:22:22Z

Thanks for taking a look @glwagner! I'll open a PR to make sure we're working with PrescribedVelocityFields correctly. And I'll also open a second PR to start looking at cubed sphere performance + getting it working on the GPU.

ali-ramadhan added 18 commits March 29, 2021 16:40

Bring back the ConformalCubedSphereGrid with proper face connectivity

4b3312b

Simple tests

53cd0c2

Merge branch 'ali/cubed-sphere-waves' into ali/full-cubed-sphere

a97b61d

set! for cubed sphere faces

4699882

Cubed sphere from file

c4bf07b

We need a ConformalCubedSphereField

ae03db3

Cubed sphere nodes

d4792e7

Plotting cubed sphere fields

eebebcf

Need cubed sphere connectivity types

666c4e1

Cubed sphere exchange boundary conditions

dc74b0e

Filling cubed sphere field halos

dfe843c

Fill the rest of the halos

a1ca92b

Pesky velocity halos on the cubed sphere

6031653

Cubed sphere kernel launching

fc57bd1

Merge branch 'master' into ali/full-cubed-sphere

136f231

Merge branch 'ali/tullio-cfl' into ali/full-cubed-sphere

2ac89e2

Start butchering cubed sphere halo filling

7ae3ac9

More invasive cubed sphere kernel launching

9c50350

francispoulin reviewed Mar 30, 2021

View reviewed changes

ali-ramadhan added 10 commits March 30, 2021 13:40

One time step!

d9886cf

More cubed sphere dispatch and simulation runs!

dd2f110

More cubed sphere goodies

d045fe7

Some cubed sphere tests to avoid catastrophic failure

722458b

Fix cubed sphere tests

20d72d9

Janky state checker

b5b9259

Fix some more kernels

333f234

Animate from file only

5019310

Nuke sandbox

0e5e44a

More useful to plot on a projection map

dee9d0f

Reorganize cubed sphere utils and grid metric halo filling

7f54272

ali-ramadhan added 3 commits April 12, 2021 20:59

More exports and set! functions

9f5e4ed

Rossby-Haurwitz validation experiment

463ab76

Flesh out Rossby-Haurwitz

3259f32

ali-ramadhan marked this pull request as ready for review April 13, 2021 13:25

ali-ramadhan added 6 commits April 13, 2021 09:32

Merge branch 'master' into ali/full-cubed-sphere

bba7680

ReducedField for cubed spheres

1f5b88a

Try Rossby-Haurwitz with CS96 grid

2b0cd44

Merge branch 'master' into ali/full-cubed-sphere

9ab8233

Fix tests

4ca3ceb

Technical debt-ridden Connected topology

94dc9a2

ali-ramadhan requested review from glwagner and christophernhill April 14, 2021 02:22

christophernhill reviewed Apr 14, 2021

View reviewed changes

Update cubed sphere validation experiments

28763e1

ali-ramadhan mentioned this pull request Apr 14, 2021

Connected topologies #1582

Closed

glwagner reviewed Apr 14, 2021

View reviewed changes

glwagner approved these changes Apr 14, 2021

View reviewed changes

ali-ramadhan merged commit d58bbaa into master Apr 14, 2021

ali-ramadhan deleted the ali/full-cubed-sphere branch April 14, 2021 15:22

ali-ramadhan mentioned this pull request Apr 14, 2021

Use PrescribedVelocityFields properly #1588

Closed

christophernhill mentioned this pull request Apr 15, 2021

Cnh/full cube implicit solver test #1593

Closed

ali-ramadhan mentioned this pull request Apr 16, 2021

Upgrade to Julia 1.6 #1514

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fully connected cubed sphere #1522

Fully connected cubed sphere #1522

ali-ramadhan commented Mar 30, 2021 •

edited

Loading

francispoulin Mar 30, 2021

ali-ramadhan Mar 31, 2021

francispoulin left a comment

ali-ramadhan commented Apr 12, 2021

ali-ramadhan commented Apr 14, 2021 •

edited

Loading

christophernhill left a comment

christophernhill Apr 14, 2021

ali-ramadhan Apr 14, 2021

christophernhill Apr 14, 2021

ali-ramadhan Apr 14, 2021

christophernhill Apr 14, 2021

christophernhill Apr 14, 2021

ali-ramadhan Apr 14, 2021

christophernhill Apr 14, 2021

ali-ramadhan Apr 14, 2021

ali-ramadhan commented Apr 14, 2021

glwagner Apr 14, 2021

glwagner Apr 14, 2021

glwagner Apr 14, 2021

glwagner Apr 14, 2021

glwagner left a comment

ali-ramadhan commented Apr 14, 2021

		@@ -68,13 +68,7 @@ A = 1e-5 * H # Amplitude of the perturbation

		η′(λ, φ, z) = A * exp(- (λ - λ₀)^2 / Δλ^2) * exp(- (φ - φ₀)^2 / Δφ^2)

Fully connected cubed sphere #1522

Fully connected cubed sphere #1522

Conversation

ali-ramadhan commented Mar 30, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

francispoulin left a comment

Choose a reason for hiding this comment

ali-ramadhan commented Apr 12, 2021

ali-ramadhan commented Apr 14, 2021 • edited Loading

christophernhill left a 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

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

ali-ramadhan commented Apr 14, 2021

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 left a comment

Choose a reason for hiding this comment

ali-ramadhan commented Apr 14, 2021

ali-ramadhan commented Mar 30, 2021 •

edited

Loading

ali-ramadhan commented Apr 14, 2021 •

edited

Loading