Skip to content

Start using columnwise #3833

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.

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 21 commits into
base: main
Choose a base branch
from
Open

Start using columnwise #3833

wants to merge 21 commits into from

Conversation

charleskawczynski
Copy link
Member

@charleskawczynski charleskawczynski commented Jun 2, 2025

@coderabbitai ignore

This PR starts using columnwise!, for the initial assignment of Yₜ = 0.

It looks like this needs a working version of CliMA/ClimaCore.jl#2343 to work, since I'm seeing (via Cthulhu):

Advanced: dump [P]arams cache.
   %3 =  = < semi-concrete eval > getproperty(::StencilBroadcasted{…},::Symbol{…})::…
   %5 =  = < semi-concrete eval > getproperty(::StencilBroadcasted{…},::Symbol{…})::…
   %6 = instantiate_args(::Tuple{…})::…
   %8 =  = < semi-concrete eval > getproperty(::StencilBroadcasted{…},::Symbol{…})::Core.Const(nothing)
   %13 = axes(::StencilBroadcasted{…})::…
 • %31 = device(::FiniteDifferenceSpace{…})::Core.Const(ClimaCore.DeviceSideDevice())

In other words, not all broadcasted objects can be instantiated on the gpu with DeviceSideDevice.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🔭 Outside diff range comments (1)
src/prognostic_equations/remaining_tendency.jl (1)

1-258: ⚠️ Potential issue

Fix formatting to pass CI check.

The JuliaFormatter CI check is failing. Please run the formatter on this file.

🧰 Tools
🪛 GitHub Actions: JuliaFormatter

[error] 1-1: Some files have not been formatted. Please format the file to pass the CI check.

🧹 Nitpick comments (1)
src/prognostic_equations/remaining_tendency.jl (1)

16-34: Consider simplifying tuple construction.

The trailing commas in the tuple construction aren't needed when items are on separate lines. Also, the splatting pattern could be clearer.

 function ᶜremaining_tendency(ᶜY, ᶠY, p, t)
     names = propertynames(ᶜY)
     tends = (
-            ᶜremaining_tendency_ρ(ᶜY, ᶠY, p, t)...,
-            ᶜremaining_tendency_uₕ(ᶜY, ᶠY, p, t)...,
-            ᶜremaining_tendency_ρe_tot(ᶜY, ᶠY, p, t)...,
-            ᶜremaining_tendency_ρq_tot(ᶜY, ᶠY, p, t)...,
-            ᶜremaining_tendency_sgsʲs(ᶜY, ᶠY, p, t)...,
+            ᶜremaining_tendency_ρ(ᶜY, ᶠY, p, t)...,
+            ᶜremaining_tendency_uₕ(ᶜY, ᶠY, p, t)...,
+            ᶜremaining_tendency_ρe_tot(ᶜY, ᶠY, p, t)...,
+            ᶜremaining_tendency_ρq_tot(ᶜY, ᶠY, p, t)...,
+            ᶜremaining_tendency_sgsʲs(ᶜY, ᶠY, p, t)...
     )
     return lazy.(prognostic_nt.(Val(names), tends...))
 end
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 660894d and da5ca1d.

📒 Files selected for processing (1)
  • src/prognostic_equations/remaining_tendency.jl (1 hunks)
🧰 Additional context used
🪛 GitHub Actions: JuliaFormatter
src/prognostic_equations/remaining_tendency.jl

[error] 1-1: Some files have not been formatted. Please format the file to pass the CI check.

⏰ Context from checks skipped due to timeout of 90000ms (11)
  • GitHub Check: ci 1.10 - macOS-latest
  • GitHub Check: ci 1.11 - ubuntu-latest
  • GitHub Check: ci 1.11 - windows-latest
  • GitHub Check: ci 1.10 - ubuntu-latest
  • GitHub Check: ci 1.10 - windows-latest
  • GitHub Check: evaluate
  • GitHub Check: test (1.11)
  • GitHub Check: test (1.10)
  • GitHub Check: downstream ClimaCoupler.jl (1.11)
  • GitHub Check: downstream ClimaCoupler.jl (1.10)
  • GitHub Check: docbuild
🔇 Additional comments (3)
src/prognostic_equations/remaining_tendency.jl (3)

13-15: Helper function looks good.

Clean implementation using Val pattern for compile-time name resolution.


36-70: Individual tendency functions are well-structured.

Consistent pattern across all functions with proper handling of optional fields.


75-87: Columnwise implementation looks correct.

The transition to device-aware columnwise operation is well implemented. Is the empty p_kernel tuple intentional or a placeholder for future parameters?

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/prognostic_equations/remaining_tendency.jl (1)

36-70: Consistent zero-tendency initialization.

All functions follow the same pattern correctly. Consider a macro to reduce boilerplate if more fields are added in the future.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between da5ca1d and fcfbe97.

📒 Files selected for processing (1)
  • src/prognostic_equations/remaining_tendency.jl (1 hunks)
🧰 Additional context used
🪛 GitHub Actions: JuliaFormatter
src/prognostic_equations/remaining_tendency.jl

[error] 1-1: Some files have not been formatted. Please format the file to pass the CI check.

⏰ Context from checks skipped due to timeout of 90000ms (11)
  • GitHub Check: ci 1.10 - ubuntu-latest
  • GitHub Check: ci 1.11 - windows-latest
  • GitHub Check: ci 1.11 - ubuntu-latest
  • GitHub Check: ci 1.10 - windows-latest
  • GitHub Check: ci 1.11 - macOS-latest
  • GitHub Check: test (1.10)
  • GitHub Check: test (1.11)
  • GitHub Check: downstream ClimaCoupler.jl (1.10)
  • GitHub Check: downstream ClimaCoupler.jl (1.11)
  • GitHub Check: evaluate
  • GitHub Check: docbuild
🔇 Additional comments (3)
src/prognostic_equations/remaining_tendency.jl (3)

13-14: LGTM!

Clean helper function for creating NamedTuples.


16-34: Good modular design for tendency collection.

The functions properly gather individual tendencies and apply lazy evaluation for performance.


75-94:

❓ Verification inconclusive

Address the formatting issue.

The columnwise implementation looks good, but the CI check is failing due to formatting.

Run JuliaFormatter to fix the formatting:


🏁 Script executed:

#!/bin/bash
# Check if JuliaFormatter.jl is available and run it
julia -e 'using Pkg; Pkg.add("JuliaFormatter"); using JuliaFormatter; format("src/prognostic_equations/remaining_tendency.jl")'

Length of output: 225


Ensure CI formatting compliance

The columnwise CUDA-aware update is spot on. CI is failing due to formatting on src/prognostic_equations/remaining_tendency.jl. Please run JuliaFormatter locally (with Julia installed) to reformat that file so the CI passes.

@charleskawczynski charleskawczynski force-pushed the ck/use_columnwise2 branch 2 times, most recently from 0d9e127 to 728413e Compare June 2, 2025 18:41
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (1)
src/prognostic_equations/remaining_tendency.jl (1)

43-43: Move import to top of file.

The using ClimaCore.RecursiveApply: rzero import should be placed with other imports at the top of the file for better organization.

+using ClimaCore.RecursiveApply: rzero

Move this line to the top with other imports and remove from line 43.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between fcfbe97 and 728413e.

📒 Files selected for processing (1)
  • src/prognostic_equations/remaining_tendency.jl (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
  • GitHub Check: evaluate
  • GitHub Check: docbuild
  • GitHub Check: ci 1.10 - windows-latest
  • GitHub Check: ci 1.10 - macOS-latest
  • GitHub Check: ci 1.11 - windows-latest
  • GitHub Check: ci 1.11 - ubuntu-latest
  • GitHub Check: ci 1.10 - ubuntu-latest
  • GitHub Check: downstream ClimaCoupler.jl (1.11)
  • GitHub Check: downstream ClimaCoupler.jl (1.10)
  • GitHub Check: test (1.11)
  • GitHub Check: test (1.10)
  • GitHub Check: format
🔇 Additional comments (5)
src/prognostic_equations/remaining_tendency.jl (5)

13-17: Clean helper function implementation.

The prognostic_nt function correctly merges zero-initialized and actual tendencies using rzero and merge.


19-42: Well-structured tendency aggregation functions.

Both ᶜremaining_tendency and ᶠremaining_tendency properly collect field-specific tendencies and apply lazy evaluation. The modular approach improves maintainability.


44-93: Consistent field tendency implementations.

The individual center field functions follow a good pattern of checking field existence and returning appropriate zero values. Note that ᶜremaining_tendency_sgsʲs uses rzero while others use zero - this appears intentional for the structured field type.


94-103: Clean face field implementations.

The face field tendency functions correctly follow the same pattern as center fields.


108-130: Excellent device-aware modernization.

The replacement of direct zeroing with Operators.columnwise! is well-implemented:

  • Proper device detection for CUDA vs CPU
  • Correct local memory flag configuration
  • Appropriate surface component handling due to columnwise! limitations
  • Clean separation of kernel parameters

This should significantly improve GPU performance.

@charleskawczynski charleskawczynski force-pushed the ck/use_columnwise2 branch 2 times, most recently from 120392e to de6d9fc Compare June 2, 2025 19:31
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (1)
src/prognostic_equations/remaining_tendency.jl (1)

115-117: Surface field handling is a temporary workaround.

The comment indicates columnwise! doesn't handle .sfc yet. Consider tracking this limitation.

Would you like me to help create an issue to track when columnwise! can fully handle surface fields?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 120392e and de6d9fc.

📒 Files selected for processing (1)
  • src/prognostic_equations/remaining_tendency.jl (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (12)
  • GitHub Check: ci 1.10 - windows-latest
  • GitHub Check: ci 1.11 - macOS-latest
  • GitHub Check: ci 1.11 - ubuntu-latest
  • GitHub Check: ci 1.10 - ubuntu-latest
  • GitHub Check: ci 1.11 - windows-latest
  • GitHub Check: downstream ClimaCoupler.jl (1.10)
  • GitHub Check: downstream ClimaCoupler.jl (1.11)
  • GitHub Check: docbuild
  • GitHub Check: test (1.10)
  • GitHub Check: test (1.11)
  • GitHub Check: evaluate
  • GitHub Check: format
🔇 Additional comments (3)
src/prognostic_equations/remaining_tendency.jl (3)

13-17: LGTM: Clean named tuple merging implementation.

The function correctly creates ordered zero-initialized tuples and merges with actual values.


44-103: LGTM: Consistent pattern for individual tendency functions.

All functions follow the same pattern: check property existence, return appropriate zero value or empty tuple. Correct use of zero() vs rzero() for different types.


19-42:

❓ Verification inconclusive

Verify lazy evaluation performance impact.

The modular approach with lazy evaluation is well-structured, but consider the performance implications of the broadcast lazy operations.


🏁 Script executed:

#!/bin/bash
# Check if there are any performance benchmarks or tests for columnwise operations
rg -A 5 -B 5 "columnwise.*benchmark|benchmark.*columnwise|@btime.*columnwise"

Length of output: 79


Add performance benchmarks for lazy broadcast operations
No existing benchmarks cover the lazy. pipelines. To ensure lazy evaluation doesn’t introduce unacceptable overhead, please add BenchmarkTools.jl tests comparing lazy vs. eager evaluation for:

  • src/prognostic_equations/remaining_tendency.jl: ᶜremaining_tendency, ᶠremaining_tendency

Use @btime on representative inputs to quantify any performance impact.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

♻️ Duplicate comments (2)
src/prognostic_equations/remaining_tendency.jl (2)

43-43: Import ordering issue persists.

The rzero import is still placed after its first use in prognostic_nt (line 14).


234-256: Device detection and columnwise call look correct.

The device-aware implementation properly handles local memory flags and applies columnwise operations efficiently.

🧹 Nitpick comments (1)
src/prognostic_equations/remaining_tendency.jl (1)

174-175: Complex lazy expression hard to debug.

The nested @. lazy() with ᶠinterp, ρ, and ᶠright_bias operations creates a complex expression that may be difficult to debug if issues arise.

Consider breaking into intermediate variables:

-water_adv(ᶜρ, ᶜJ, ᶠJ, ᶜχ) =
-    @. lazy(ᶜprecipdivᵥ(ᶠinterp(ρ * ᶜJ) / ᶠJ * ᶠright_bias(-(ᶜχ))))
+function water_adv(ᶜρ, ᶜJ, ᶠJ, ᶜχ)
+    ᶠρJ = @. lazy(ᶠinterp(ᶜρ * ᶜJ) / ᶠJ)
+    ᶠχ_bias = @. lazy(ᶠright_bias(-ᶜχ))
+    return @. lazy(ᶜprecipdivᵥ(ᶠρJ * ᶠχ_bias))
+end
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between de6d9fc and 0dc554a.

📒 Files selected for processing (2)
  • src/prognostic_equations/remaining_tendency.jl (1 hunks)
  • src/prognostic_equations/water_advection.jl (0 hunks)
💤 Files with no reviewable changes (1)
  • src/prognostic_equations/water_advection.jl
⏰ Context from checks skipped due to timeout of 90000ms (13)
  • GitHub Check: docbuild
  • GitHub Check: ci 1.10 - windows-latest
  • GitHub Check: test (1.11)
  • GitHub Check: ci 1.11 - ubuntu-latest
  • GitHub Check: ci 1.10 - macOS-latest
  • GitHub Check: ci 1.11 - macOS-latest
  • GitHub Check: ci 1.10 - ubuntu-latest
  • GitHub Check: test (1.10)
  • GitHub Check: evaluate
  • GitHub Check: downstream ClimaCoupler.jl (1.10)
  • GitHub Check: ci 1.11 - windows-latest
  • GitHub Check: downstream ClimaCoupler.jl (1.11)
  • GitHub Check: format
🔇 Additional comments (1)
src/prognostic_equations/remaining_tendency.jl (1)

13-17: Function logic is sound.

The prognostic_nt function correctly creates ordered zero-initialized tendencies and merges with provided values.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (4)
src/prognostic_equations/remaining_tendency.jl (4)

13-17: Import ordering issue persists.

The rzero import at line 43 still occurs after its first use in prognostic_nt at line 14.

Also applies to: 43-43


44-55: Missing variable definition in ᶜremaining_tendency_ρ.

The function references ᶜρ in the water_adv call (line 52) but never defines it. Compare with ᶜremaining_tendency_ρq_tot which properly defines ᶜρ = ᶜY.ρ at line 105.


66-86: Missing variable definitions in ᶜremaining_tendency_ρe_tot.

The water_adv call at line 85 references ᶜJ and ᶠJ which aren't defined in this function scope.

+    ᶜJ = Fields.local_geometry_field(ᶜY).J  
+    ᶠJ = Fields.local_geometry_field(ᶠY).J
     if !(p.atmos.moisture_model isa DryModel)
         ᶜwₕhₜ = compute_ᶜwₕhₜ(ᶜY, ᶠY, p, t)
         ∑tendencies = lazy.(∑tendencies .- water_adv(ᶜρ, ᶜJ, ᶠJ, ᶜwₕhₜ))

234-256: Surface field workaround needs documentation.

The manual .sfc handling due to columnwise! limitations should be documented in the function docstring.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0dc554a and 8d2ded8.

📒 Files selected for processing (4)
  • src/cache/precipitation_precomputed_quantities.jl (1 hunks)
  • src/prognostic_equations/advection.jl (0 hunks)
  • src/prognostic_equations/remaining_tendency.jl (1 hunks)
  • src/prognostic_equations/water_advection.jl (0 hunks)
💤 Files with no reviewable changes (2)
  • src/prognostic_equations/water_advection.jl
  • src/prognostic_equations/advection.jl
⏰ Context from checks skipped due to timeout of 90000ms (12)
  • GitHub Check: ci 1.11 - ubuntu-latest
  • GitHub Check: ci 1.11 - macOS-latest
  • GitHub Check: ci 1.10 - macOS-latest
  • GitHub Check: ci 1.10 - windows-latest
  • GitHub Check: ci 1.10 - ubuntu-latest
  • GitHub Check: ci 1.11 - windows-latest
  • GitHub Check: docbuild
  • GitHub Check: test (1.10)
  • GitHub Check: test (1.11)
  • GitHub Check: downstream ClimaCoupler.jl (1.10)
  • GitHub Check: downstream ClimaCoupler.jl (1.11)
  • GitHub Check: evaluate
🔇 Additional comments (5)
src/cache/precipitation_precomputed_quantities.jl (3)

32-63: LGTM - Well-structured precipitation velocity computation.

The function properly extracts microphysics parameters and computes terminal velocities for all hydrometeor types. The max(zero(...), ...) pattern correctly ensures non-negative mixing ratios.


66-82: Good multiple dispatch pattern for water advection terms.

The default zero implementation and specialized computation for NonEquilMoistModel + Microphysics1Moment follow the established pattern. The weighted sum calculation correctly incorporates all hydrometeor contributions.


84-100: Energy advection implementation looks correct.

The inclusion of thermodynamic internal energies, geopotential, and kinetic energy terms via the Kin helper is consistent with existing patterns. The lazy evaluation approach is appropriate for performance.

src/prognostic_equations/remaining_tendency.jl (2)

189-205: Well-structured thermodynamic state computation.

The compute_ᶜts function properly handles all necessary thermodynamic variables with appropriate lazy evaluation.


216-230: Good boundary condition handling for velocity computations.

The functions correctly handle surface velocity reconstruction and boundary conditions. The type assertions help catch potential issues early.

@charleskawczynski charleskawczynski force-pushed the ck/use_columnwise2 branch 3 times, most recently from aaeb6aa to 76c6307 Compare June 3, 2025 15:04
@haakon-e
Copy link
Member

haakon-e commented Jun 3, 2025

I haven't been fully looped into this from the start, so I have some basic questions before I can effectively review this.

Could you please add some more context for the changes. Is there an issue / write-up I could read that helps me understand what you're proposing?

Quickly looking through the diff, I can only see columnwise! added once. What are the other changes?

I would find it useful if the functions have docstrings and if there existed some docs and/or developer docs explaining how others can follow this pattern for their work.

@charleskawczynski
Copy link
Member Author

I haven't been fully looped into this from the start, so I have some basic questions before I can effectively review this.

Could you please add some more context for the changes. Is there an issue / write-up I could read that helps me understand what you're proposing?

Yes, here is a pretty comprehensive writeup motivating this: #3594.

Quickly looking through the diff, I can only see columnwise! added once. What are the other changes?

Correct. columnwise! will launch one kernel, compute and store several tendencies in Yₜ.

I would find it useful if the functions have docstrings and if there existed some docs and/or developer docs explaining how others can follow this pattern for their work.

I completely agree. That is a big part where I'd like feedback (and where some of these functions should live, and how to compose them). I may break this PR into multiple parts. The first commit passes CI in that all of the experiments are reproduced, there are some allocations but they're not egregious and I would recommend just bumping the allocation limits for now.

If the first commit looks okay, it would make my life easier to merge that first, and then we could add more tendencies separately in independent PRs.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

♻️ Duplicate comments (3)
src/prognostic_equations/remaining_tendency.jl (3)

44-55: Missing variable definition (duplicate issue).

The undefined ᶜρ variable issue in line 52 has already been identified in past reviews.


83-86: Undefined variables (duplicate issue).

The missing ᶜJ and ᶠJ definitions for the water advection call have already been identified in past reviews.


234-261: Solid device-aware refactor.

The device detection and columnwise! integration look correct. The .sfc workaround handling is appropriate.

🧹 Nitpick comments (1)
.buildkite/Manifest-v1.11.toml (1)

407-412: Pin ClimaDiagnostics dependency and ensure reproducibility
You've updated ClimaDiagnostics to a new commit hash and bumped the version to 0.2.15, adding repo-rev and repo-url. Please confirm this revision corresponds to an official release tag. Consider pinning to a specific version tag (e.g., v0.2.15) instead of main to avoid future breaking changes and ensure reproducible builds.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between aaeb6aa and 33a79cb.

📒 Files selected for processing (5)
  • .buildkite/Manifest-v1.11.toml (1 hunks)
  • src/cache/precipitation_precomputed_quantities.jl (1 hunks)
  • src/prognostic_equations/advection.jl (0 hunks)
  • src/prognostic_equations/remaining_tendency.jl (1 hunks)
  • src/prognostic_equations/water_advection.jl (0 hunks)
💤 Files with no reviewable changes (2)
  • src/prognostic_equations/water_advection.jl
  • src/prognostic_equations/advection.jl
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/cache/precipitation_precomputed_quantities.jl
⏰ Context from checks skipped due to timeout of 90000ms (12)
  • GitHub Check: test (1.10)
  • GitHub Check: test (1.11)
  • GitHub Check: evaluate
  • GitHub Check: downstream ClimaCoupler.jl (1.10)
  • GitHub Check: downstream ClimaCoupler.jl (1.11)
  • GitHub Check: docbuild
  • GitHub Check: ci 1.10 - windows-latest
  • GitHub Check: ci 1.11 - windows-latest
  • GitHub Check: ci 1.10 - macOS-latest
  • GitHub Check: ci 1.10 - ubuntu-latest
  • GitHub Check: ci 1.11 - ubuntu-latest
  • GitHub Check: ci 1.11 - macOS-latest
🔇 Additional comments (8)
src/prognostic_equations/remaining_tendency.jl (8)

13-17: Clean helper function design.

Good approach to ensure complete field coverage by merging zero-initialized defaults with actual tendency values.


19-34: Excellent modular architecture.

The decomposition into individual tendency functions improves maintainability and the lazy evaluation should optimize performance.


35-42: Consistent face field implementation.

Good consistency with the center field pattern and covers the expected face field variables.


56-65: Correct momentum tendency implementation.

Proper handling of viscous and Rayleigh sponge tendencies with appropriate conditional checks.


100-125: Good variable scoping pattern.

This function demonstrates the correct approach for defining ᶜJ, ᶠJ, and ᶜρ variables that should be replicated in other tendency functions.


126-145: Appropriate placeholder implementations.

Simple zero tendency returns are correct for these water species variables at this stage of development.


146-171: Correct SGS tendency handling.

Appropriate use of rzero for SGS field initialization and good consistency between center and face implementations.


177-231: Well-designed utility functions.

The helper functions for velocity computation, thermodynamic state, and type assertions provide good support for the main tendency calculations.

ᶠz = Fields.coordinate_field(axes(ᶠu₃)).z
sfc_u₃ = surface_velocity_full(ᶠu₃, ᶠuₕ³)
# todo: generalize this with z_min
return @. lazy(ifelse(iszero(ᶠz), sfc_u₃, ᶠu₃))
Copy link
Member

Choose a reason for hiding this comment

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

ᶠz is non-zero if we include orography. e.g.

julia> Fields.level(ᶠz, Utilities.half) |> extrema
(0.0f0, 2332.788f0)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we need to think of a generalized way to do this.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

♻️ Duplicate comments (2)
src/prognostic_equations/remaining_tendency.jl (2)

52-63: Missing variable definition - see past comment.

The undefined ᶜρ variable issue was already flagged. This still needs to be addressed.


261-263: Surface field workaround needs documentation.

This limitation was already flagged in past comments and needs proper documentation.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 449299c and 820efac.

📒 Files selected for processing (1)
  • src/prognostic_equations/remaining_tendency.jl (1 hunks)
🔇 Additional comments (12)
src/prognostic_equations/remaining_tendency.jl (12)

13-22: Well-designed generated function for NamedTuple reordering.

Clean implementation that efficiently reorders NamedTuple fields at compile time.


24-25: Simple wrapper function is clear and concise.

Good abstraction that delegates to the more general sorted_nt function.


43-50: Face field tendencies look correct.

The structure mirrors the center field function appropriately.


65-68: Smart helper functions for tendency operations.

The null broadcast handling is a nice touch for robust computation.


70-79: Clean implementation of horizontal velocity tendencies.

Properly handles both viscous and Rayleigh sponge contributions.


80-113: Complex but well-structured energy tendency function.

The conditional logic for moisture models and upwinding is correctly implemented.


114-140: Total water tendency implementation looks solid.

Handles both moisture model checks and tracer upwinding appropriately.


141-186: Simple tendency functions for hydrometeor species.

Most return zero tendencies appropriately. The SGS functions correctly use rzero for recursive structures.


189-191: Water advection function is now correctly implemented.

The variable naming issue was already fixed in a previous commit.


222-229: Type assertion utilities are well-designed.

Good defensive programming with clear error messages.


249-276: Device-aware columnwise implementation is excellent.

The conditional logic for CUDA vs CPU memory handling is well-structured.


27-42:

❓ Verification inconclusive

Verify all center field tendency functions exist.

The function calls multiple ᶜremaining_tendency_* functions. Ensure all referenced functions are properly implemented and tested.


🏁 Script executed:

#!/bin/bash
# Verify all referenced center tendency functions are defined
ast-grep --pattern 'function ᶜremaining_tendency_$_($_) $_'

Length of output: 61


Let’s try ripgrep to locate all definitions:


🏁 Script executed:

#!/bin/bash
# Search entire repo for any remaining_tendency_* function definitions
rg -n 'function\s+ᶜremaining_tendency_' .

Length of output: 1058


All center-field tendency functions are implemented
I verified that all ten ᶜremaining_tendency_* functions called by ᶜremaining_tendency exist in src/prognostic_equations/remaining_tendency.jl. Please ensure each has adequate test coverage.

@CliMA CliMA deleted a comment from coderabbitai bot Jun 4, 2025
@charleskawczynski charleskawczynski force-pushed the ck/use_columnwise2 branch 2 times, most recently from 9948680 to 70508a3 Compare June 4, 2025 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants