Skip to content

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

Merged
imreddyTeja merged 2 commits intomainfrom
tr/bc-follow
Apr 23, 2026
Merged

Fix eager eval of non-Field broadcasts in calc_level_val#2494
imreddyTeja merged 2 commits intomainfrom
tr/bc-follow

Conversation

@imreddyTeja
Copy link
Copy Markdown
Member

@imreddyTeja imreddyTeja commented Apr 21, 2026

The finite difference evaluation on GPUs incorrectly handles broadcasts that are not <: AbstractFieldStyle when they are nested within another Broadcasted.

  • Code follows the style guidelines OR N/A.
  • Unit tests are included OR N/A.
  • Code is exercised in an integration test OR N/A.
  • Documentation has been added/updated OR N/A.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Fixes CUDA finite-difference eager evaluation so that nested Broadcasted expressions that are not field-style (<: AbstractFieldStyle) are no longer handled as if they were field broadcasts when computing calc_level_val.

Changes:

  • Import AbstractFieldStyle for use in CUDA eager-eval dispatch.
  • Narrow reconstruct_space_and_call_calc_level_val’s special-case dispatch to Broadcasted{<:AbstractFieldStyle} to avoid reconstructing placeholder spaces for non-field-style nested broadcasts.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread ext/cuda/operators_fd_eager.jl
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@@ -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.
Comment thread .buildkite/pipeline.yml
Comment on lines +1195 to +1203
- 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

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.
@imreddyTeja imreddyTeja marked this pull request as ready for review April 22, 2026 23:30
Copy link
Copy Markdown
Member

@dennisYatunin dennisYatunin left a comment

Choose a reason for hiding this comment

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

I've tested the changes in this branch against #2417. The end-to-end test breaks when that PR is rebased off main, but it works when that PR is rebased off this branch.

@imreddyTeja imreddyTeja merged commit 0365344 into main Apr 23, 2026
40 checks passed
@imreddyTeja imreddyTeja deleted the tr/bc-follow branch April 23, 2026 20:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants