Add space with multiple columns by ph-kev · Pull Request #2525 · CliMA/ClimaCore.jl

ph-kev · 2026-06-10T18:51:56Z

This PR adds a space that can supports multiple columns. This is needed to complete CliMA/ClimaLand.jl#1723.

This PR have been tested with the FluxnetSimulations in ClimaLand and it produces identical results with the current single column used for the FluxnetSimulation.

The remapping feature will not be done in this PR. For the purpose of calibration, this is not needed because the data is extracted from the ClimaCore fields rather than from the NetCDF files produced from the diagnostics.

Note that most of the code in this PR have been written with Claude.

nefrathenrici

This looks good, I only saw a few immediate correctness issues.

imreddyTeja · 2026-06-15T18:52:56Z


+# PointCloudGrid has no quadrature_style field; pass nothing so that FD
+# operators on PointColumnEnsembleSpace work on CUDA.
+Adapt.adapt_structure(


Would it be better to combine this with the above method

Adapt.adapt_structure( to::CUDA.KernelAdaptor, grid::Grids.ExtrudedFiniteDifferenceGrid, ) = Grids.DeviceExtrudedFiniteDifferenceGrid( Adapt.adapt(to, Grids.vertical_topology(grid)), hasproperty(grid, :horizontal_grid.quadrature_style), Adapt.adapt(to, grid.horizontal_grid.quadrature_style), Adapt.adapt(to, grid.global_geometry), Adapt.adapt(to, grid.center_local_geometry), Adapt.adapt(to, grid.face_local_geometry), )

imreddyTeja · 2026-06-15T19:14:55Z


 function ExtrudedFiniteDifferenceGrid(
-    horizontal_grid::Union{SpectralElementGrid1D, SpectralElementGrid2D},
+    horizontal_grid::Union{SpectralElementGrid1D, SpectralElementGrid2D, PointCloudGrid},


This union appears a lot. Could it just be a const or abstract type?

Yeah aren't these all subtypes of AbstractSpectralElementGrid? Could just use that instead.

imreddyTeja · 2026-06-15T19:16:16Z

+diagonal matrix `diag(R·π/180, R·cosd(lat)·π/180)` reflecting the sphere metric at
+each point, and `J = R²·cosd(lat)·(π/180)²`.
+"""
+struct PointCloudGrid{


I think it might be better to add a field for topology, and make it always nothing.

This could simplify all the topology(grid) methods, but subsequent operations on the result of topology would still be invalid. We could make some sort of NoTopology struct, or add methods to topology-related functions so they can handle Nothing, but that feels roughly equivalent to what this PR is doing anyway. So I don't think adding a field for the topology would be very useful.

dennisYatunin

This looks great! It has a fair bit of code duplication, both in terms of method definitions and const type declarations, but that's a higher-level design issue that can be fixed separately. I left a few nitpicks about readability and error handling, but overall it looks ready to merge in.

dennisYatunin · 2026-06-26T22:31:30Z

+    @assert context isa ClimaComms.SingletonCommsContext "PointColumnEnsembleGrid only supports SingletonCommsContext."
+    @assert ClimaComms.device(context) == device "The given device and context device do not match."


Suggested change

@assert context isa ClimaComms.SingletonCommsContext "PointColumnEnsembleGrid only supports SingletonCommsContext."

@assert ClimaComms.device(context) == device "The given device and context device do not match."

context = ClimaComms.SingletonCommsContext(device)

No need to provide a keyword argument for context if it can only have one possible value.

dennisYatunin · 2026-06-26T22:39:11Z


 function ExtrudedFiniteDifferenceGrid(
-    horizontal_grid::Union{SpectralElementGrid1D, SpectralElementGrid2D},
+    horizontal_grid::Union{SpectralElementGrid1D, SpectralElementGrid2D, PointCloudGrid},


Yeah aren't these all subtypes of AbstractSpectralElementGrid? Could just use that instead.

dennisYatunin · 2026-06-26T22:43:54Z

+# For PointCloudGrid there is no horizontal topology; route context/device
+# through the horizontal grid directly (which stores its own context).
+ClimaComms.context(grid::ExtrudedFiniteDifferenceGrid{<:PointCloudGrid}) =
+    ClimaComms.context(grid.horizontal_grid)
+ClimaComms.device(grid::ExtrudedFiniteDifferenceGrid{<:PointCloudGrid}) =
+    ClimaComms.device(grid.horizontal_grid)


Suggested change

# For PointCloudGrid there is no horizontal topology; route context/device

# through the horizontal grid directly (which stores its own context).

ClimaComms.context(grid::ExtrudedFiniteDifferenceGrid{<:PointCloudGrid}) =

ClimaComms.context(grid.horizontal_grid)

ClimaComms.device(grid::ExtrudedFiniteDifferenceGrid{<:PointCloudGrid}) =

ClimaComms.device(grid.horizontal_grid)

ClimaComms.context(grid::ExtrudedFiniteDifferenceGrid) =

ClimaComms.context(grid.horizontal_grid)

ClimaComms.device(grid::ExtrudedFiniteDifferenceGrid) =

ClimaComms.device(grid.horizontal_grid)

This could also be the default for every ExtrudedFiniteDifferenceGrid. Special cases for such specific subtypes are generally harder to maintain.

dennisYatunin · 2026-06-26T23:00:02Z

+The `local_geometry` is stored as an `IFH{LG, 1, N}` data layout — one node per
+"element" (`Ni = 1`), N "elements" (`Nh = N`). The horizontal Jacobian `∂x∂ξ` is the
+diagonal matrix `diag(R·π/180, R·cosd(lat)·π/180)` reflecting the sphere metric at
+each point, and `J = R²·cosd(lat)·(π/180)²`.


Suggested change

The `local_geometry` is stored as an `IFH{LG, 1, N}` data layout — one node per

"element" (`Ni = 1`), N "elements" (`Nh = N`). The horizontal Jacobian `∂x∂ξ` is the

diagonal matrix `diag(R·π/180, R·cosd(lat)·π/180)` reflecting the sphere metric at

each point, and `J = R²·cosd(lat)·(π/180)²`.

The `local_geometry` is stored as an `IFH{LG, 1, N}` data layout, with each of

the `N` locations represented by an element with one nodal point. Based on the

[metric tensor](https://en.wikipedia.org/wiki/Metric_tensor#The_round_metric_on_a_sphere)

of a sphere, the horizontal Jacobian `∂x∂ξ` is given by the diagonal matrix

`diag(R·π/180, R·cosd(lat)·π/180)`, with the determinant `J = R²·cosd(lat)·(π/180)²`.

Wording here was a bit unclear, and I thought it would be good to add a link to some reference.

dennisYatunin · 2026-06-26T23:03:37Z

+diagonal matrix `diag(R·π/180, R·cosd(lat)·π/180)` reflecting the sphere metric at
+each point, and `J = R²·cosd(lat)·(π/180)²`.
+"""
+struct PointCloudGrid{


This could simplify all the topology(grid) methods, but subsequent operations on the result of topology would still be invalid. We could make some sort of NoTopology struct, or add methods to topology-related functions so they can handle Nothing, but that feels roughly equivalent to what this PR is doing anyway. So I don't think adding a field for the topology would be very useful.

dennisYatunin · 2026-06-26T23:08:39Z

+# No topology — return nothing. Callers that need a topology (e.g. DSS) should
+# not be called on a PointCloudGrid.
+topology(::PointCloudGrid) = nothing


Could this throw an error instead? Otherwise, users will see a MethodError about how something does not work when the topology is Nothing, rather than a more informative message about how they should not access the topology of a PointCloudGrid.

dennisYatunin · 2026-06-26T23:09:50Z

+    device::ClimaComms.AbstractDevice = ClimaComms.device(),
+    context::ClimaComms.AbstractCommsContext = ClimaComms.SingletonCommsContext(device),
+) where {FT}
+    @assert context isa ClimaComms.SingletonCommsContext "PointCloudGrid only supports SingletonCommsContext."


Suggested change

@assert context isa ClimaComms.SingletonCommsContext "PointCloudGrid only supports SingletonCommsContext."

context = ClimaComms.SingletonCommsContext(device)

As in CommonGrids, you can avoid the keyword argument if it only accepts one possible value.

dennisYatunin · 2026-06-26T23:16:47Z

+
+`local_geometry` holds an `IFH{LG, 1, N}` data layout.
+"""
+struct PointCloudLevelSpace{


Can this be called a PointCloudSpace instead? The name of every other ___Space type matches that of its corresponding ___Grid type.

dennisYatunin · 2026-06-26T23:20:00Z

+set_mask!(fn, space::MultiColumnFiniteDifferenceSpace) =
+    set_mask!(fn, grid(space).horizontal_grid)


Shouldn't this do nothing if get_mask always returns NoMask?

dennisYatunin · 2026-06-26T23:22:27Z

+    return size(level_space.local_geometry, 5) == size(
+        Grids.local_geometry_data(hgrid, nothing), 5)


Could this check the local_geometry itself, instead of just its size? Maybe something like level_space.local_geometry === Grids.local_geometry_data(hgrid, nothing), if that's possible.

ph-kev force-pushed the kp/multiple-columns branch 6 times, most recently from 7567810 to e78f8a4 Compare June 10, 2026 23:20

ph-kev requested review from dennisYatunin, imreddyTeja and nefrathenrici June 10, 2026 23:49

nefrathenrici reviewed Jun 12, 2026

View reviewed changes

Comment thread test/TestUtilities/TestUtilities.jl Outdated

Comment thread src/Grids/pointcloud.jl Outdated

Comment thread src/Spaces/extruded.jl Outdated

ph-kev marked this pull request as ready for review June 12, 2026 17:53

Add space with multiple columns

02c2eb2

ph-kev force-pushed the kp/multiple-columns branch from e78f8a4 to 02c2eb2 Compare June 12, 2026 20:28

imreddyTeja approved these changes Jun 18, 2026

View reviewed changes

dennisYatunin approved these changes Jun 26, 2026

View reviewed changes

This was referenced Jul 6, 2026

v1 Release Checklist CliMA/ClimaDiagnostics.jl#183

Open

Add support for multiple columns CliMA/ClimaUtilities.jl#230

Open

		@assert context isa ClimaComms.SingletonCommsContext "PointColumnEnsembleGrid only supports SingletonCommsContext."
		@assert ClimaComms.device(context) == device "The given device and context device do not match."

	@assert context isa ClimaComms.SingletonCommsContext "PointColumnEnsembleGrid only supports SingletonCommsContext."
	@assert ClimaComms.device(context) == device "The given device and context device do not match."
	context = ClimaComms.SingletonCommsContext(device)

-The `local_geometry` is stored as an `IFH{LG, 1, N}` data layout — one node per
-"element" (`Ni = 1`), N "elements" (`Nh = N`). The horizontal Jacobian `∂x∂ξ` is the
-diagonal matrix `diag(R·π/180, R·cosd(lat)·π/180)` reflecting the sphere metric at
-each point, and `J = R²·cosd(lat)·(π/180)²`.
+The `local_geometry` is stored as an `IFH{LG, 1, N}` data layout, with each of
+the `N` locations represented by an element with one nodal point. Based on the
+[metric tensor](https://en.wikipedia.org/wiki/Metric_tensor#The_round_metric_on_a_sphere)
+of a sphere, the horizontal Jacobian `∂x∂ξ` is given by the diagonal matrix
+`diag(R·π/180, R·cosd(lat)·π/180)`, with the determinant `J = R²·cosd(lat)·(π/180)²`.

		set_mask!(fn, space::MultiColumnFiniteDifferenceSpace) =
		set_mask!(fn, grid(space).horizontal_grid)

		return size(level_space.local_geometry, 5) == size(
		Grids.local_geometry_data(hgrid, nothing), 5)

Uh oh!

Conversation

ph-kev commented Jun 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nefrathenrici left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dennisYatunin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dennisYatunin Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dennisYatunin Jun 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ph-kev commented Jun 10, 2026 •

edited

Loading

dennisYatunin Jun 26, 2026 •

edited

Loading

dennisYatunin Jun 26, 2026 •

edited

Loading