Skip to content

Fix eager eval of non-Field broadcasts in calc_level_val #2494

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

Merged
imreddyTeja merged 2 commits into from
Apr 23, 2026
+44 −2
Merged

Fix eager eval of non-Field broadcasts in calc_level_val #2494

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
84db859
Fix eager eval of non-Field broadcasts in calc_level_val
imreddyTeja Apr 21, 2026
3d18a61
Add test for stencil with non AbstractFieldStyle broadcasts
imreddyTeja Apr 22, 2026
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
9 changes: 9 additions & 0 deletions .buildkite/pipeline.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1192,6 +1192,15 @@ steps:
slurm_gpus: 1
slurm_mem: 10GB

- label: "Unit: matrix field broadcasting (GPU)"
key: unit_matrix_field_broadcasting_gpu_scalar_17
command: "julia --color=yes --check-bounds=yes --project=.buildkite test/MatrixFields/matrix_fields_broadcasting/test_scalar_17.jl"
env:
CLIMACOMMS_DEVICE: "CUDA"
agents:
slurm_gpus: 1
slurm_mem: 10GB

Comment on lines +1195 to +1203
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

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

test_scalar_17.jl is only wired into the GPU broadcasting CI group. The CPU broadcasting group currently runs scalar_1–scalar_16, so this addition makes the CPU/GPU scalar test lists diverge. If this test is not truly GPU-only, consider adding a matching CPU step (or consolidating these lists) to keep coverage symmetric and avoid missing regressions on CPU.

Suggested change
- label: "Unit: matrix field broadcasting (GPU)"
key: unit_matrix_field_broadcasting_gpu_scalar_17
command: "julia --color=yes --check-bounds=yes --project=.buildkite test/MatrixFields/matrix_fields_broadcasting/test_scalar_17.jl"
env:
CLIMACOMMS_DEVICE: "CUDA"
agents:
slurm_gpus: 1
slurm_mem: 10GB

Copilot uses AI. Check for mistakes.
# non-scalar
- label: "Unit: matrix field broadcasting (GPU)"
key: unit_matrix_field_broadcasting_gpu_non_scalar_1
Expand Down
4 changes: 2 additions & 2 deletions ext/cuda/operators_fd_eager.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import ClimaCore: Spaces, Quadratures, Topologies, Operators
import Base.Broadcast: Broadcasted
import ClimaCore.Fields: Field, field_values
import ClimaCore.Fields: Field, field_values, AbstractFieldStyle
import ClimaComms
import ClimaCore.Utilities: half
import ClimaCore.Operators
Expand Down Expand Up @@ -204,7 +204,7 @@ Base.@propagate_inbounds reconstruct_space_and_call_calc_level_val(
arg::A,
space::S,
) where {
A <: Union{Base.Broadcast.Broadcasted, StencilBroadcasted, Field},
A <: Union{Base.Broadcast.Broadcasted{<:AbstractFieldStyle}, StencilBroadcasted, Field},
S,
} = @inbounds @inline calc_level_val(arg, reconstruct_placeholder_space(axes(arg), space))
Comment thread
imreddyTeja marked this conversation as resolved.
Base.@propagate_inbounds reconstruct_space_and_call_calc_level_val(
Expand Down
33 changes: 33 additions & 0 deletions test/MatrixFields/matrix_fields_broadcasting/test_scalar_17.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
#=
julia --project
using Revise; include(joinpath("test", "MatrixFields", "matrix_fields_broadcasting", "test_scalar_4.jl"))
Copy link

Copilot AI Apr 22, 2026

Choose a reason for hiding this comment

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

The Revise/include snippet in the header comment still references test_scalar_4.jl. This makes it harder to run this test directly; update it to point to test_scalar_17.jl (and keep the path consistent with other scalar tests).

Suggested change
using Revise; include(joinpath("test", "MatrixFields", "matrix_fields_broadcasting", "test_scalar_4.jl"))
using Revise; include(joinpath("test", "MatrixFields", "matrix_fields_broadcasting", "test_scalar_17.jl"))

Copilot uses AI. Check for mistakes.
=#
import ClimaCore
#! format: off
include(joinpath(pkgdir(ClimaCore),"test","MatrixFields","matrix_fields_broadcasting","test_scalar_utils.jl"))
#! format: on
test_opt = get(ENV, "BUILDKITE", "") == "true"
@testset "tri-diagonal matrix times Diagonal with nested broadcasts" begin
bc = @lazy @. ᶠᶠmat * DiagonalMatrixRow(((1.0f0 - 0.5f0 * 2.0f0 + 0.0f0 * ᶠvec)))
result = materialize(bc)

input_fields = (ᶠᶠmat, ᶠvec)
ref_set_result! = (_result, _ᶠᶠmat, _ᶠvec) -> _result .= 0.0f0

unit_test_field_broadcast_vs_array_reference(
result,
bc;
input_fields,
ref_set_result!,
using_cuda,
allowed_max_eps_error = 10,
)
test_opt && opt_test_field_broadcast_against_array_reference(
result,
bc;
input_fields,
ref_set_result!,
using_cuda,
)
test_opt && !using_cuda && perf_getidx(bc)
end
Loading