Use bucket cache e_per_area
#1304
Conversation
WalkthroughThe changes remove the Assessment against linked issues
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
juliasloan25
force-pushed
the
js/bucket-energy
branch
from
dffec7a to
bb931f4
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.buildkite/pipeline.yml(1 hunks)experiments/ClimaEarth/Manifest-v1.11.toml(2 hunks)experiments/ClimaEarth/components/land/climaland_bucket.jl(1 hunks)experiments/ClimaEarth/components/land/climaland_helpers.jl(1 hunks)experiments/ClimaEarth/setup_run.jl(0 hunks)
💤 Files with no reviewable changes (1)
- experiments/ClimaEarth/setup_run.jl
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: ci 1.11 - windows-latest
- GitHub Check: ci 1.11 - macOS-latest
- GitHub Check: ci 1.11 - ubuntu-latest
- GitHub Check: ci 1.10 - windows-latest
- GitHub Check: ci 1.10 - macOS-latest
- GitHub Check: test (1.11)
- GitHub Check: ci 1.10 - ubuntu-latest
- GitHub Check: test (1.10)
- GitHub Check: docbuild
🔇 Additional comments (3)
experiments/ClimaEarth/Manifest-v1.11.toml (1)
351-353: Pinned commit vs moving branch – please double-check reproducibility
repo-rev = "tr/add-energy_per_area-bucket"points to a branch.
Although the manifest also pins the exactgit-tree-sha1, the CI job (see pipeline.yml) later installsClimaLandby branch only, which will fetch the latest HEAD and can diverge from this commit, silently breaking reproducibility.Consider either
- repo-rev = "tr/add-energy_per_area-bucket" + repo-rev = "d5ba1073aa5c015f6d25ee9ace265d63ac421197" # exact commitor drop the extra
Pkg.addin the pipeline so the manifest alone governs the version.experiments/ClimaEarth/components/land/climaland_helpers.jl (1)
54-56: Good addition, just verify API availability
CL.Domains.obtain_face_spaceis used to derivesubsurface_face.
Looks fine, but double-check that this helper exists in all supportedClimaLand
versions; otherwise gate it behind a version check to avoidUndefVarError.experiments/ClimaEarth/components/land/climaland_bucket.jl (1)
239-240: Simplified energy retrieval LGTM – make sure cache is kept up-to-dateDirectly exposing
sim.integrator.p.bucket.e_per_arearemoves redundant
re-computations and should be faster.
Action item: ensuree_per_areais updated every RHS call in the new
ClimaLand branch; otherwise the coupler will report stale values.
| - "julia --project=experiments/ClimaEarth/ -e 'using Pkg; Pkg.add(Pkg.PackageSpec(;name=\"ClimaLand\", rev=\"tr/add-energy_per_area-bucket\"))'" | ||
| - "julia --project=experiments/ClimaEarth/ -e 'using Pkg; Pkg.precompile()'" |
There was a problem hiding this comment.
Non-deterministic package install may override the manifest
Pkg.add(Pkg.PackageSpec(;name="ClimaLand", rev="tr/add-energy_per_area-bucket"))
re-pulls the current branch tip every run and can mismatch the commit hashed in
Manifest-v1.11.toml, defeating reproducible CI.
If the intent is to stay on the commit already in the manifest, this line can be removed entirely.
If you really need a refresh, pin the exact commit hash instead of the branch:
- Pkg.add(Pkg.PackageSpec(;name="ClimaLand", rev="tr/add-energy_per_area-bucket"))
+ Pkg.add(Pkg.PackageSpec(;name="ClimaLand", rev="d5ba1073aa5c015f6d25ee9ace265d63ac421197"))
juliasloan25
force-pushed
the
js/bucket-energy
branch
from
bb931f4 to
6d7480d
Compare
Purpose
CliMA/ClimaLand.jl#1104 adds
e_per_areato the Bucket cache, independent of whether conservation checks are enabled. This allows us to access that field directly and simplify the coupler code. It's tested out here.closes #1261
To-do
p.e_per_areadirectly inget_field(::BucketSimulation, Val(:energy))get_new_cachemethod for Bucketenergy_checkto Bucket constructor