Add support for Broadcasted objects
#64
Conversation
|
Could we instead use |
|
Yes, my goal is to eventually allow for kernel fusion (which is what I meant with "pathway for future improvement"). However, I don't know how to use Is it just a matter of adding Maybe this is a good opportunity to add some user documentation to |
I'm happy to add more docs to Using LazyBroadcast, using LazyBroadcast: @lazy
compute_ta(state, cache, time) = @lazy @. state.taand the result could later be used as |
|
Thank you! I got something to work and I managed to cluster all the
As for documentation: I am looking at LazyBroadcasts.jl and all the docs I see is the readme which does not mention This is what I think could be improved in the readme:
Here are few things I tried to get the code to work: julia> import LazyBroadcast: @lazy
julia> compute_ta(state, cache, time) = @lazy @. state.ta
ERROR: LoadError: Invalid expression given to materialize_args
Stacktrace:
[1] error(s::String)
@ Base ./error.jl:35
[2] materialize_args(expr::Expr)
@ LazyBroadcast ~/.julia/packages/LazyBroadcast/SVKU8/src/utils.jl:8
[3] transform(e::Expr)
@ LazyBroadcast ~/.julia/packages/LazyBroadcast/SVKU8/src/LazyBroadcast.jl:30 [4] _lazy_broadcasted(expr::Expr)
@ LazyBroadcast ~/.julia/packages/LazyBroadcast/SVKU8/src/LazyBroadcast.jl:43 [5] var"@lazy"(__source__::LineNumberNode, __module__::Module, expr::Any)
@ LazyBroadcast ~/.julia/packages/LazyBroadcast/SVKU8/src/LazyBroadcast.jl:54in expression starting at REPL[17]:1
julia> compute_ta(state, cache, time) = @lazy @. state
ERROR: LoadError: type Symbol has no field args
Stacktrace:
[1] getproperty(x::Symbol, f::Symbol)
@ Base ./Base.jl:37
[2] code_info(expr::Expr)
@ LazyBroadcast ~/.julia/packages/LazyBroadcast/SVKU8/src/code_lowered_single_expression.jl:10
[3] code_lowered_single_expression(expr::Expr)
@ LazyBroadcast ~/.julia/packages/LazyBroadcast/SVKU8/src/code_lowered_single_expression.jl:12
[4] transform(e::Expr)
@ LazyBroadcast ~/.julia/packages/LazyBroadcast/SVKU8/src/LazyBroadcast.jl:29 [5] _lazy_broadcasted(expr::Expr)
@ LazyBroadcast ~/.julia/packages/LazyBroadcast/SVKU8/src/LazyBroadcast.jl:43 [6] var"@lazy"(__source__::LineNumberNode, __module__::Module, expr::Any)
@ LazyBroadcast ~/.julia/packages/LazyBroadcast/SVKU8/src/LazyBroadcast.jl:54in expression starting at REPL[18]:1I also found that these do not work: julia> a, b = [1], [2]
julia> @lazy a .= b
Expr
head: Symbol .=
args: Array{Any}((2,))
1: Symbol a
2: Symbol b
dump(expr) = nothing
Expr
head: Symbol .=
args: Array{Any}((2,))
1: Symbol a
2: Symbol b
dump(expr) = nothing
ERROR: LoadError: Uncaught edge case
Stacktrace:
[1] error(s::String)
@ Base ./error.jl:35
[2] check_restrictions(expr::Expr)
@ LazyBroadcast ~/.julia/packages/LazyBroadcast/SVKU8/src/utils.jl:48
[3] _lazy_broadcasted(expr::Expr)
@ LazyBroadcast ~/.julia/packages/LazyBroadcast/SVKU8/src/LazyBroadcast.jl:42 [4] var"@lazy"(__source__::LineNumberNode, __module__::Module, expr::Any)
@ LazyBroadcast ~/.julia/packages/LazyBroadcast/SVKU8/src/LazyBroadcast.jl:54in expression starting at REPL[12]:1
julia> @lazy @dot a = b
Expr
head: Symbol macrocall
args: Array{Any}((3,))
1: Symbol @dot
2: LineNumberNode
line: Int64 1
file: Symbol REPL[19]
3: Expr
head: Symbol =
args: Array{Any}((2,))
1: Symbol a
2: Symbol b
dump(expr) = nothing
Expr
head: Symbol macrocall
args: Array{Any}((3,))
1: Symbol @dot
2: LineNumberNode
line: Int64 1
file: Symbol REPL[19]
3: Expr
head: Symbol =
args: Array{Any}((2,))
1: Symbol a
2: Symbol b
dump(expr) = nothing
ERROR: LoadError: Uncaught edge case
Stacktrace:
[1] error(s::String)
@ Base ./error.jl:35
[2] check_restrictions(expr::Expr)
@ LazyBroadcast ~/.julia/packages/LazyBroadcast/SVKU8/src/utils.jl:48
[3] _lazy_broadcasted(expr::Expr)
@ LazyBroadcast ~/.julia/packages/LazyBroadcast/SVKU8/src/LazyBroadcast.jl:42 [4] var"@lazy"(__source__::LineNumberNode, __module__::Module, expr::Any)
@ LazyBroadcast ~/.julia/packages/LazyBroadcast/SVKU8/src/LazyBroadcast.jl:54in expression starting at REPL[19]:1Eventually I found that it works with From here, I think I understand that the correct function to write is compute_ta(out, state, cache, time) = @lazy @. out = state.taThis seems to work in my integration test case. |
Great question. That's safe to do so long the data passed into the broadcasted objects point to the same memory. That said, it's actually very cheap to do this because all it's doing is making an instance of a struct with a bunch of pointers, so I recommend we reconstruct the object every time to be safe.
We may be able to use
Yeah, the
Yeah, I agree on all of these points. LazyBroadcast.jl was born because it was originally a bunch of utilities in MultiBroadcastFusion.jl, and I realized that they were more generally useful, so I split those utilities off into a separate package. I hadn't fully thought out the use-cases, and if/what pieces could be made more flexible (like the broken examples you showed below), so I wasn't sure how to best document things. I'll open an issue with these points.
Yeah, this is a bit technical I think, and it does seem that there are some sharp edges (I didn't expect that last example to fail). It might be easier to discuss some of this over zoom. One tricky aspect of this is that, for example,
Yeah, this will work, and that's how I'd expect it be used. There might be something we can do to remove needing |
Sbozzolo
force-pushed
the
gb/assign
branch
3 times, most recently
from
ba077ff to
34bf23f
Compare
Sbozzolo
changed the title
Broadcasted objects
|
Passing ClimaAtmos build: https://buildkite.com/clima/climaatmos-ci/builds/20044 I did not change the EDMF and radiation diagnostics because they are much more complex. The allocation flame graphs are failing because I added more for loops. |
Nice! Do we still need |
I am not sure. Does Fields.array2field(
cache.radiation.rrtmgp_model.face_sw_flux_dn,
axes(state.f),
)? |
For something like that, we don't even need x = Fields.array2field(
cache.radiation.rrtmgp_model.face_sw_flux_dn,
axes(state.f),
)
bc = Base.broadcasted(identity, x) |
|
In fact, any diagnostic that simply returns a field, could just wrap it in |
|
Waiting for |
Sbozzolo
force-pushed
the
gb/assign
branch
2 times, most recently
from
cd7e940 to
93ec15f
Compare
Sbozzolo
changed the title
Broadcasted objectsBroadcasted objects
Sbozzolo
force-pushed
the
gb/assign
branch
3 times, most recently
from
540752b to
1c7f4ef
Compare
Sbozzolo
force-pushed
the
gb/assign
branch
2 times, most recently
from
6d08d4b to
d237e8f
Compare
|
Warning You have reached your daily quota limit. As a reminder, free tier users are limited to 5 requests per day. Please wait up to 24 hours and I will start processing your requests again! |
5 similar comments
|
Warning You have reached your daily quota limit. As a reminder, free tier users are limited to 5 requests per day. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning You have reached your daily quota limit. As a reminder, free tier users are limited to 5 requests per day. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning You have reached your daily quota limit. As a reminder, free tier users are limited to 5 requests per day. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning You have reached your daily quota limit. As a reminder, free tier users are limited to 5 requests per day. Please wait up to 24 hours and I will start processing your requests again! |
|
Warning You have reached your daily quota limit. As a reminder, free tier users are limited to 5 requests per day. Please wait up to 24 hours and I will start processing your requests again! |
I will wait to merge this: I want to fully debug the current issue with CUDA 5.7 before introducing more changes into the diagnostics stack |
WalkthroughThis pull request integrates the Changes
Assessment against linked issues
Possibly related issues
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (9)
NEWS.md (1)
19-22: Consider mentioning performance & memory benefits.Highlight the advantages of using un-evaluated expressions, such as reduced memory allocation or improved kernel fusion, to emphasize why
LazyBroadcastmay be beneficial.src/clima_diagnostics.jl (2)
61-65: Enhance the docstring with usage details.Highlight the preference order: if
compute!is defined, it uses in-place, otherwise it falls back tocompute. This clarifies behavior for new users.
85-87: Potential scalability concern.This custom
uniquemethod has O(N^2) complexity. For large diagnostic sets, consider more efficient deduplication strategies (e.g., hashing or set membership).docs/src/user_guide.md (4)
54-59: Clarify the need for copying
The documentation statescopyis needed to avoid unintended modifications tostate.ta. However, the examplecompute_tafunction on lines 40–41 does not actually callcopy. Consider updating the example or clarifying why a direct return withoutcopyis acceptable here, to prevent confusion.
62-62: Use fenced code blocks
This code block is indented rather than fenced, triggering a markdown style warning. Switch to fenced code blocks (e.g., ```julia) to match project styling guidelines.- function compute_ta!(out, state, cache, time) - if isnothing(out) - return state.ta - ... +```julia +function compute_ta!(out, state, cache, time) + if isnothing(out) + return state.ta + ...🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
62-62: Code block style
Expected: fenced; Actual: indented(MD046, code-block-style)
99-99: Fix grammatical error
Replace “should be compute:” with “should be computed:”- should be compute: Using `LazyBroadcast.jl`, ... + should be computed: Using `LazyBroadcast.jl`, ...🧰 Tools
🪛 LanguageTool
[grammar] ~99-~99: There may an error in the verb form ‘be compute’.
Context: ...t a recipe on how the diagnostic should be compute: UsingLazyBroadcast.jl, the snippet ...(MD_BE_NON_VBP)
109-109: Add a missing comma
Add a comma after “efficiently” to avoid ambiguity.- knows how to handle it efficiently avoiding the intermediate allocations. + knows how to handle it efficiently, avoiding the intermediate allocations.🧰 Tools
🪛 LanguageTool
[uncategorized] ~109-~109: Possible missing comma found.
Context: ...limaDiagnostics` knows how to handle it efficiently avoiding the intermediate allocations. ...(AI_HYDRA_LEO_MISSING_COMMA)
src/DiagnosticVariables.jl (1)
39-41: Correct spelling of LazyBroadcast
The docstring references “LazyBoardcast” instead of “LazyBroadcast”.- (e.g., with `LazyBoardcast.lazy`). + (e.g., with `LazyBroadcast.lazy`).docs/src/developer_guide.md (1)
240-245: Guidance on Leveraging LazyBroadcast
The added instructions emphasize using LazyBroadcast.jl to avoid intermediate allocations. For clarity, consider rephrasing “avoid intermediate allocations” to “prevent unnecessary temporary allocations.”🧰 Tools
🪛 LanguageTool
[style] ~243-~243: Consider a more expressive alternative.
Context: ... and avoid intermediate allocations. To do that, addLazyBroadcastto your depen...(DO_ACHIEVE)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.buildkite/Project.toml(1 hunks)NEWS.md(3 hunks)Project.toml(2 hunks)docs/src/developer_guide.md(7 hunks)docs/src/index.md(1 hunks)docs/src/user_guide.md(2 hunks)docs/src/writers.md(1 hunks)src/DiagnosticVariables.jl(2 hunks)src/clima_diagnostics.jl(4 hunks)test/diagnostic_variable.jl(1 hunks)test/integration_test.jl(4 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/src/user_guide.md
[uncategorized] ~90-~90: Possible missing comma found.
Context: ...yBroadcast.jl)). Consider the following example where we want to shift the temperature ...
(AI_HYDRA_LEO_MISSING_COMMA)
[grammar] ~99-~99: There may an error in the verb form ‘be compute’.
Context: ...t a recipe on how the diagnostic should be compute: Using LazyBroadcast.jl, the snippet ...
(MD_BE_NON_VBP)
[uncategorized] ~109-~109: Possible missing comma found.
Context: ...limaDiagnostics` knows how to handle it efficiently avoiding the intermediate allocations. ...
(AI_HYDRA_LEO_MISSING_COMMA)
docs/src/developer_guide.md
[style] ~243-~243: Consider a more expressive alternative.
Context: ... and avoid intermediate allocations. To do that, add LazyBroadcast to your depen...
(DO_ACHIEVE)
🪛 markdownlint-cli2 (0.17.2)
docs/src/user_guide.md
62-62: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: downstream ClimaCoupler.jl
- GitHub Check: downstream ClimaCoupler.jl
- GitHub Check: downstream ClimaLand.jl
- GitHub Check: downstream ClimaLand.jl
- GitHub Check: test (1.11)
- GitHub Check: downstream ClimaAtmos.jl
- GitHub Check: downstream ClimaAtmos.jl
🔇 Additional comments (33)
.buildkite/Project.toml (1)
15-15: Added LazyBroadcast dependency as required.The addition of LazyBroadcast as a dependency is appropriate and aligns with the PR objective of supporting lazy evaluation for diagnostic variables. This enables the new
computefunction to returnBase.Broadcast.Broadcastedobjects created using the@lazymacro.docs/src/index.md (1)
27-28: Documentation updated with new LazyBroadcast featureThe added bullet point clearly documents the new support for lazy expressions using LazyBroadcast.jl. This is an essential documentation update that reflects the changes in functionality introduced by this PR.
test/diagnostic_variable.jl (2)
25-29: Good error testing for mutually exclusive compute functionsThis test correctly verifies that an error is thrown when both
computeandcompute!functions are provided to the DiagnosticVariable constructor, enforcing that they should be mutually exclusive.
31-32: Proper validation for required compute functionThis test ensures that the DiagnosticVariable constructor throws an error when neither
computenorcompute!is provided, verifying that at least one compute function is required.docs/src/writers.md (1)
44-44: Method name corrected for consistencyThe method name has been updated from
FakePressureLevelMethod()toFakePressureLevelsMethod()to ensure consistency with the actual method name referenced in line 91. This correction improves the accuracy of the documentation.Project.toml (3)
27-27: Consider specifying a version range forLazyBroadcast.Pinning it to
"1"may limit flexibility and could introduce constraints. You might consider a compatible range (e.g.,LazyBroadcast = "1"up to<2.0) if feasible.
45-45: Verify correct UUID usage.Confirm that
"9dccce8e-a116-406d-9fcc-a88ed4f510c8"is indeed the current UUID associated withLazyBroadcast.jl, ensuring mismatches do not cause resolution issues.
52-52: Check that tests requireLazyBroadcast.Since
LazyBroadcastis now included in test dependencies, verify that existing or upcoming tests actually use this package, and ensure coverage is robust.NEWS.md (3)
17-18: Good descriptive heading.This heading clearly delineates the new
lazycapability introduced in this release.
23-32: Legacy snippet is sufficiently explained.The example for the old in-place approach provides good clarity on the motivation for the new feature.
33-46: Great illustration of the newlazypattern.This clear example demonstrates how returning a lazy expression simplifies code. Examples for both broadcasted calculations and Fields are well-written.
src/clima_diagnostics.jl (5)
125-137: Well-documented dispatch logic.The explanatory comments for the three cases (in-place compute, straightforward compute, lazy broadcast compute) are clear and match the code flow.
138-139: In-place computation detection is fine.
has_inplace_compute = !isnothing(variable.compute!)works well to differentiate compute methods without complicating introspection logic.
140-152: Robust branching for multiple compute paths.This snippet correctly handles both in-place and broadcast-based computations. It’s concise and aligns with your design goal.
240-263: Consistent orchestration logic.The code correctly mirrors earlier logic, materializing broadcasted results or invoking
compute!in place, and storing them accurately.
266-270: Time reduction flow looks solid.Nothing appears problematic in applying the stored reduction function and ensuring data is successfully accumulated.
docs/src/user_guide.md (2)
40-41: Straightforward function example
These lines correctly illustrate a minimalcomputefunction returningstate.ta.
50-50:✅ Verification successful
Confirm downstream usage
compute = compute_talooks fine, but please ensure all references tocompute_taalign with this new function signature throughout the codebase.Run the following script to identify calls to
compute_taand confirm none rely on the old in-place usage:
🏁 Script executed:
#!/bin/bash rg -A 5 "compute_ta"Length of output: 2846
Downstream Usage Verified – No Further Changes Required
Based on our review, the usage of
compute_tain the assignmentcompute = compute_ta(as seen indocs/src/user_guide.mdat line 50) is consistent with the updated function signature. Our search results confirmed that across the repository—including the definitions in bothNEWS.mdanddocs/src/user_guide.md—all references tocompute_ta(and its in-place variantcompute_ta!) align with the intended behavior. Please double-check any specific downstream callers if needed, but no discrepancies were found.src/DiagnosticVariables.jl (1)
70-85: Constructor logic looks good
Requiring that exactly one ofcomputeorcompute!is provided is a clean and safe approach. No further issues found.test/integration_test.jl (5)
70-72: Lazy broadcasting usage
Returninglazy.(u.my_var .+ 10.0)properly defers the computation. This aligns well with lazy evaluation best practices.
74-76: Simple field-based function
Returningu.my_vardirectly is correct for a non-lazy approach.
84-88: Definition ofsimple_var_lazy
No issues found. This diagnostic variable setup appears consistent with the newcomputedesign.
90-94: Definition ofsimple_var_field
Likewise looks good. Uses the straightforward, field-basedcomputemethod.
107-128:❓ Verification inconclusive
Expanded diagnostics coverage
Adding lazy and field-based variants to scheduled diagnostics helps thoroughly test both paths. Confirm that regression tests fully validate correct output, especially for the lazy approach.You can run the existing integration tests to ensure they pass and produce expected results:
Also applies to: 153-156
🏁 Script executed:
#!/bin/bash # Ensure all tests pass, focusing on integration_test.jl # This relies on the project’s typical test harness commands # (not provided in your snippet), for example: # julia --project=. -e 'using Pkg; Pkg.test()' # or a specialized test script if present. echo "Please run the project’s test suite to verify correctness."Length of output: 126
Verify Integration Tests and Diagnostic Outputs
The changes introducing both lazy and field-based scheduled diagnostics expand our coverage. However, please confirm that the regression tests fully validate the expected behavior—especially for the lazy variant. In particular, ensure that:
- The integration tests in
test/integration_test.jl(lines 107–128 and 153–156) produce the correct outputs.- The implementations for
ClimaDiagnostics.ScheduledDiagnostic(along withaverage_pre_output_hook!and the divisor-based schedule) behave as intended under both lazy and field conditions.- Running the full test suite (e.g., via
julia --project=. -e 'using Pkg; Pkg.test()') confirms that these paths are correctly exercised.After verifying these aspects, please update the review accordingly.
docs/src/developer_guide.md (9)
108-108: Example Lambda: Confirming NewcomputeSignature
The lambda now correctly adheres to the new signature(u, p, t), returning the appropriate diagnostic field. Ensure that all similar examples across the documentation follow this updated pattern.
146-151: Parameter Signature Update: UsingcomputeInstead ofcompute!
The function signature foradd_diagnostic_variable!has been updated to acceptcompute(without the exclamation mark) rather thancompute!. This aligns with the new evaluation strategy. Make sure that all related references in the documentation and code are updated accordingly.
176-179: Enhanced Documentation forcomputeParameter
The docstring now clearly specifies that thecomputefunction should return aFieldor aBase.Broadcast.Broadcastedexpression and must avoid allocating newFieldobjects. This clarification supports the intended lazy evaluation and performance improvement.
181-188: Updated Function Definition inadd_diagnostic_variable!
The updated function definition now acceptscomputeas a keyword argument. This change is consistent with the new design direction and improves clarity regarding the expected function behavior.
204-204: Forwarding thecomputeFunction
Thecomputeparameter is correctly forwarded when constructing theDiagnosticVariableobject. This ensures that the variable is configured with the updated lazy or direct computation logic as intended.
232-239: Diagnostic Example forrhoaUsing NewcomputeFormat
The example for the air density diagnostic now correctly uses thecomputefunction in place ofcompute!. This is in line with the PR objectives.
247-258: Lazy Evaluation Example forrhoaDiagnostic
This example demonstrates how to apply lazy broadcasting using thelazymacro. It clearly shows how to implement a more complex expression while avoiding intermediate allocations. Make sure that users are reminded to importlazyfrom LazyBroadcast in their code.
278-285: Implementation ofcompute_huswith Lazy Evaluation
The function that accepts a moisture model parameter now returns the specific humidity using lazy broadcasting. This implementation aligns with the new evaluation strategy provided that the ambiguity in the overloaded methods is resolved.
287-294: Diagnostic Addition for Specific Humidity (hus)
The registration of the diagnostic variable for specific humidity now uses the updatedcomputeparameter (compute = compute_hus). Once thecompute_husoverload ambiguity is addressed, this example should function as intended.
`LazyBroadcast.jl` provides a way to return an unevaluated function. This is useful in two cases: 1. reduce code verbosity to handle the `isnothing(out)` case 2. allow clustering all the broadcasted expressions in a single place In turn, 2. is useful because it is the first step in fusing different broadcasted calls. This commit adds support for such functions.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
docs/src/developer_guide.md (4)
108-108: Example Update: NewcomputeSignature in DiagnosticVariable InitializationThe example now shows initializing a
DiagnosticVariableusing a lambda with three parameters(u, p, t), reflecting the updated signature that returns aFieldor aBase.Broadcast.Broadcastedexpression. This update is clear and concise.Consider adding a brief inline comment (or linking to further documentation) that reminds users this new convention replaces the previous in-place
compute!approach.
176-179: Enhanced Documentation for thecomputeParameterThe updated documentation now clearly specifies that the
computefunction must return aFieldor aBase.Broadcast.Broadcastedexpression and should not allocate newFieldobjects. This guidance is valuable for users optimizing performance with lazy evaluation.Consider elaborating slightly on what constitutes a “Field” (or linking to a definition) and why using dot operations is a hint to switch to lazy evaluation.
241-246: Updated Guidance on Lazy Evaluation with LazyBroadcastThe revised text now advises users to leverage LazyBroadcast.jl to avoid intermediate allocations during computation. This is a clear and useful update.
Consider rephrasing “avoid intermediate allocations” to “prevent unnecessary intermediate memory allocations” for enhanced clarity.
🧰 Tools
🪛 LanguageTool
[style] ~243-~243: Consider a more expressive alternative.
Context: ... and avoid intermediate allocations. To do that, addLazyBroadcastto your depen...(DO_ACHIEVE)
247-258: Practical Example of Lazy Evaluation in a Diagnostic VariableThe example for the "rhoa" diagnostic clearly demonstrates using lazy broadcast (
lazy.(...)) to process the computation expression. This not only reinforces the new paradigm but also provides a practical reference for users.It might be helpful to include a short inline note explaining why the multiplication by
1000is applied—whether it’s a unit conversion or a simulation-specific scaling—to enhance the example’s instructional value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
.buildkite/Project.toml(1 hunks)NEWS.md(3 hunks)Project.toml(2 hunks)docs/src/developer_guide.md(7 hunks)docs/src/index.md(1 hunks)docs/src/user_guide.md(2 hunks)src/DiagnosticVariables.jl(2 hunks)src/clima_diagnostics.jl(4 hunks)test/diagnostic_variable.jl(1 hunks)test/integration_test.jl(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/src/index.md
- .buildkite/Project.toml
- Project.toml
- src/clima_diagnostics.jl
- test/diagnostic_variable.jl
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
docs/src/user_guide.md
61-61: Code block style
Expected: fenced; Actual: indented
(MD046, code-block-style)
🪛 LanguageTool
docs/src/developer_guide.md
[style] ~243-~243: Consider a more expressive alternative.
Context: ... and avoid intermediate allocations. To do that, add LazyBroadcast to your depen...
(DO_ACHIEVE)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: downstream ClimaCoupler.jl
- GitHub Check: downstream ClimaCoupler.jl
- GitHub Check: downstream ClimaLand.jl
- GitHub Check: downstream ClimaLand.jl
- GitHub Check: test (1.11)
- GitHub Check: downstream ClimaAtmos.jl
- GitHub Check: climadiagnostics (1.11)
- GitHub Check: downstream ClimaAtmos.jl
- GitHub Check: test (1.10)
- GitHub Check: climadiagnostics (1.10)
🔇 Additional comments (17)
NEWS.md (1)
17-46: Great addition of the lazy evaluation feature documentation!The documentation clearly explains the new functionality allowing for un-evaluated expressions using LazyBroadcast.jl. The examples effectively demonstrate how the new
computefunction simplifies the diagnostic variable specification compared to the previouscompute!approach.docs/src/user_guide.md (4)
40-42: The new simplified compute function signature is a nice improvement.The updated function signature removes the need for output management, making the code more straightforward.
50-50: Updated DiagnosticVariable constructor to use the new compute function.This correctly implements the changes needed to support the new API.
85-109: Excellent explanation of the benefits of lazy evaluation.The documentation effectively explains why returning un-evaluated expressions with LazyBroadcast.jl is more efficient than allocating full Fields, with clear examples of both approaches.
59-84:❓ Verification inconclusive
Good backward compatibility documentation.
The compatibility notice clearly explains the differences between the new
computefunction and the legacycompute!function, which is important for users upgrading from earlier versions.
🏁 Script executed:
#!/bin/bash # Verify that the markdown code block is properly formatted # Check if any indented code blocks exist in the documentation rg -n "^\s{4,}```" docs/src/Length of output: 116
Action Required: Verify Markdown Verification Script
The backward compatibility section in
docs/src/user_guide.mdis clearly written and effectively explains the difference betweencomputeandcompute!. However, the shell script used to verify the Markdown code block formatting produced an error due to unescaped backticks. It appears that the error:/.coderabbit_commands_96e691e5-ebd4-4fdc-abd5-b7b3d0021e04.sh: line 5: unexpected EOF while looking for matching ``'is caused by Bash interpreting the triple backticks as the start of a command substitution.
Recommendations:
Update the verification command to prevent unintended command substitution (for example, use single quotes instead of double quotes):
rg -n '^\s{4,}```' docs/src/Re-run the updated command to manually confirm that the Markdown code blocks in the documentation are correctly formatted.
Please verify the updated script’s output manually to ensure that no formatting errors are present.
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
61-61: Code block style
Expected: fenced; Actual: indented(MD046, code-block-style)
test/integration_test.jl (5)
17-17: Properly importing the LazyBroadcast dependency.The code correctly imports the
lazyfunction from LazyBroadcast.
70-76: Good implementation of compute functions for testing.The test includes two different compute function implementations:
compute_my_var_lazy- correctly uses lazy broadcasting to defer evaluationcompute_my_var_field- simply returns the field without modificationThis covers both use cases of the new API.
84-94: Well-defined test DiagnosticVariables using the new compute function.The test creates diagnostic variables using the new
computefunction properly, with different variants for lazy evaluation and direct field return.
107-128: Comprehensive testing of scheduled diagnostics with lazy evaluation.The test creates various scheduled diagnostics using both the lazy and field-based variants, testing both instantaneous values and time averages.
153-156: Complete test coverage by adding new diagnostics to the scheduled list.All the new diagnostic types are properly added to the scheduled_diagnostics array, ensuring they'll be executed during the test.
src/DiagnosticVariables.jl (4)
27-31: Clear compatibility notice for the new feature.The compatibility notice clearly indicates that support for the
computefunction was introduced in version 0.2.13.
35-42: Well-documented function parameters in the docstring.The docstring has been updated to clearly explain both the
compute!andcomputeparameters, including their expected signatures and return types.
57-68: Good implementation of the DiagnosticVariable struct with optional functions.The struct has been updated to support both the new
computeand legacycompute!functions as Union{Function, Nothing} types, providing flexibility while maintaining backward compatibility.
70-96: Robust constructor with proper validation.The constructor ensures that exactly one of
computeorcompute!is provided usingxor, which is an elegant way to enforce this requirement. An appropriate error message is raised if this condition is not met.docs/src/developer_guide.md (3)
146-151: Signature Update: Transition fromcompute!tocomputein add_diagnostic_variable!The function signature for
add_diagnostic_variable!now reflects the removal of the in-place computation (compute!) in favor of the newcomputefunction. This change is in line with the lazy evaluation approach described in the PR.It would help to document any backward-compatibility considerations or migration instructions for users familiar with the old interface.
187-188: Parameter List Consistency in add_diagnostic_variable!Including
computein the function’s keyword arguments is now consistent with the new functionality.No further action required, as the update is straightforward.
293-293: Correct Associative Binding for Specific Humidity DiagnosticAssigning
compute = compute_husfor the "hus" diagnostic is now consistent with the updated interface, ensuring that the diagnostic variable correctly uses the new computation logic.
| compute_hus(state, cache, time) = | ||
| compute_hus(state, cache, time, cache.atmos.moisture_model) | ||
|
|
||
| compute_hus!(out, state, cache, time) = | ||
| compute_hus!(out, state, cache, time, cache.model.moisture_model) | ||
| compute_hus!(_, _, _, _, model::T) where {T} = | ||
| compute_hus(state, cache, time) = | ||
| compute_hus!(state, cache, time, cache.model.moisture_model) | ||
| compute_hus(_, _, _, model::T) where {T} = |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential Ambiguity in compute_hus Overloads
There are now two definitions for compute_hus(state, cache, time):
- One dispatches with
cache.atmos.moisture_model. - The other dispatches with
cache.model.moisture_model(using the legacycompute_hus!in the call).
This duality may be confusing and could lead to ambiguity if both cache.atmos and cache.model exist.
Recommendation: Consider consolidating these definitions or providing clearer dispatch conditions (or renaming one of the methods) to avoid potential conflicts in method resolution. Documentation clarifying when each branch is chosen would also be beneficial.
|
🎉🎉🎉 |
This PR adds support for specifying diagnostics with a
computefunction instead ofcompute!.compute(state, cache, time)can return either aFieldor aBase.Broadcast.Broadcastedobject (obtained with@lazy) and are now the recommended way to specify diagnostics.compute!is still supported to preserve existing code.Closes #5
Closes #103
Summary by CodeRabbit
New Features
Bug Fixes
DiagnosticVariableconstructor to ensure proper function requirements.Documentation
computefunction.