Skip to content

Tr/minor fixes #2347

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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Tr/minor fixes #2347

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d91a0be
Fix field2array error message
imreddyTeja Jun 13, 2025
410eaf9
Add tests for level function on Fields
imreddyTeja Jun 13, 2025
d3a76c4
Remove `@inbounds` from `level` function
imreddyTeja Jun 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/Fields/Fields.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,7 +517,7 @@ Base.@propagate_inbounds function level(
v::PlusHalf,
)
hspace = level(axes(field), v)
@inbounds data = level(field_values(field), v.i + 1)
data = level(field_values(field), v.i + 1)
Field(data, hspace)
end

Expand Down Expand Up @@ -664,7 +664,7 @@ element type of the array is the same as the struct type of `field`.
"""
function field2array(field::Field)
if sizeof(eltype(field)) != sizeof(eltype(parent(field)))
f_axis_size = sizeof(eltype(parent(field))) ÷ sizeof(eltype(field))
f_axis_size = sizeof(eltype(field)) ÷ sizeof(eltype(parent(field)))
error("unable to use field2array because each Field element is \
represented by $f_axis_size array elements (must be 1)")
end
Expand Down
1 change: 1 addition & 0 deletions src/Spaces/Spaces.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import ..Grids:
quadrature_style

import ClimaComms
import UnrolledUtilities: unrolled_any
using StaticArrays, ForwardDiff, LinearAlgebra, Adapt

"""
Expand Down
7 changes: 7 additions & 0 deletions src/Spaces/finitedifference.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,3 +145,10 @@ Base.@propagate_inbounds function level(
local_geometry = level(local_geometry_data(space), v)
PointSpace(ClimaComms.context(space), local_geometry)
end
function issubspace(subspace::PointSpace, space::FiniteDifferenceSpace)
all_local_geometry = local_geometry_data(space)
unrolled_any(
v -> subspace.local_geometry === level(all_local_geometry, v),
1:nlevels(space),
)
Comment on lines +150 to +153
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that we want to unroll over all nlevels for this. Isn't it sufficient to check the first level? When would it not be?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought the local geometry can vary spatially. For example:

julia> DataLayouts.level(Spaces.local_geometry_data(space), 1)[].coordinates
ZPoint(0.5)

julia> DataLayouts.level(Spaces.local_geometry_data(space), 3)[].coordinates
ZPoint(2.5)

julia> Fields.local_geometry_field(Fields.level(space, 1)).coordinates
ClimaCore.Geometry.ZPoint{Float64}-valued Field:
  z: [0.5]

julia> Fields.local_geometry_field(Fields.level(space, 3)).coordinates
ClimaCore.Geometry.ZPoint{Float64}-valued Field:
  z: [2.5]

Copy link
Member

@charleskawczynski charleskawczynski Jun 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I suppose it's safe to do the comparison over every point (even checking the range is still subject errors). But, we still probably don't want to unroll this call, since the result is not compile time known and nlevels can be quite large.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess this method is not really needed. Its only use in src is to check broadcasting shapes, which already has unique methods for PointSpaces.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I don't think it's likely that we'll fall into such an error, but it is an extra layer of protection.

On the one hand, I don't think that this can be easily determined, with our current design, at compile-time. Ideally, we could add some sort of compile-time switches into our code that would allow / enforce these checks at runtime, and we could use it as a debugging feature. But it might be a hefty amount of work to make that feature available.

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not quite sure why I used unrolling here... do you think I should just remove this functionality, or leave it without unrolling? The only argument I have for keeping it is that if it exists for the extruded spectral element spaces, I'd expect it to also exist for finite difference spaces.

Copy link
Member

@charleskawczynski charleskawczynski Jun 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I think the performance is likely fine without the unrolling, since the === comparison is likely pretty fast.

The only issue that I could see is that this operation is not compile-time known, and the logic of where this is called includes a check that the spaces are compatible (we error if they're not). If the check is not compile-time known, then it won't work inside a cuda kernel (this check is currently performed before the kernel is launched, but that won't be the case and it will need to work inside a cuda kernel with my columnwise! design, so there may be clashes with that).

end
40 changes: 40 additions & 0 deletions test/Fields/unit_field.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1196,6 +1196,46 @@ end
bf_new = @. bf # test copy()
end

@testset "Level function on fields" begin
FT = Float32
extruded_center_space = ExtrudedCubedSphereSpace(
FT;
z_elem = 10,
z_min = 0,
z_max = 1,
radius = 10,
h_elem = 10,
n_quad_points = 4,
staggering = Grids.CellCenter(),
)
fd_center_space = ColumnSpace(
FT;
z_elem = 10,
z_min = 0,
z_max = 10,
staggering = CellCenter(),
)
for center_space in (extruded_center_space, fd_center_space)
face_space = Spaces.face_space(center_space)
center_field = fill(FT(1), center_space)
face_field = fill(FT(2), face_space)
for i in 1:10
center_face_level = Fields.level(center_field, i)
face_level = Fields.level(face_field, PlusHalf(i))
@test axes(center_face_level) == Spaces.level(center_space, i)
@test Spaces.issubspace(axes(center_face_level), center_space)
@test axes(face_level) == Spaces.level(face_space, PlusHalf(i))
@test Spaces.issubspace(axes(face_level), face_space)
end
if Base.JLOptions().check_bounds == 1
@test_throws BoundsError Fields.level(center_field, 11)
@test_throws BoundsError Fields.level(face_field, PlusHalf(11))
else
@warn "Bounds check on level(::Field) not verified."
end
end
end

include("unit_field_multi_broadcast_fusion.jl")

nothing
Loading