Add energy, water to bucket cache #1104
Conversation
|
@imreddyTeja we have some functionality for this recently added: total_energy_per_area! and total_liq_water_vol_per_area! |
I don't think those functions are extended for the bucket though. It seems like there are 2 options for how we can move this forward to remove
I slightly prefer allocating space for it in the bucket cache, so we can access that field directly instead of allocating or using temp fields in the coupler |
|
@juliasloan25 that works! We can add the variables to the bucket cache and update them in ClimaLand using the extended methods of total_energy_per_area! (for the bucket) |
|
@imreddyTeja do you want to continue working on this, or do you want me to take it from here? |
I'm fine with either. If I do it, I might need a bit of guidance on synchronizing the changes between ClimaLand and the coupler. Also a very minor thing: right now the field is called |
juliasloan25
force-pushed
the
tr/add-energy_per_area-bucket
branch
2 times, most recently
from
4648238 to
1fd5811
Compare
WalkthroughThis change extends the Assessment against linked issues
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (13)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/standalone/Bucket/Bucket.jl (1)
584-614: Energy calculation implementation is correct.The function properly computes total energy by integrating soil thermal energy and subtracting snow energy. Consider adding units (J/m²) to the docstring for clarity.
-the total energy per unit ground area for the `BucketModel`. +the total energy per unit ground area (J/m²) for the `BucketModel`.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/standalone/Bucket/Bucket.jl(5 hunks)
🔇 Additional comments (4)
src/standalone/Bucket/Bucket.jl (4)
323-324: New auxiliary type looks good.Adding the floating-point type (FT) to the auxiliary_types tuple correctly supports the new e_per_area variable.
339-340: Variable naming is consistent.Adding e_per_area to auxiliary_vars aligns with project's naming conventions and PR objectives.
355-356: Domain assignment is appropriate.The :surface domain for e_per_area is consistent with similar surface-related variables.
503-504: Implementation location is logical.The placement of the e_per_area computation at the end of update_aux! is appropriate as it depends on previously updated state.
juliasloan25
force-pushed
the
tr/add-energy_per_area-bucket
branch
2 times, most recently
from
2cef12d to
0152549
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/standalone/Bucket/Bucket.jl (1)
609-614: Unused parametert– minor tidy-up.
total_energy_per_area!(and the water counterpart) ignoret. Consider prefixing it with_or removing it to silence unused-variable lint.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/standalone/Bucket/Bucket.jl(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- 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: ci 1.10 - ubuntu-latest
- GitHub Check: docbuild
- GitHub Check: test (1.11)
- GitHub Check: test (1.10)
- GitHub Check: downstream ClimaCoupler.jl (1.11)
- GitHub Check: downstream ClimaCoupler.jl (1.10)
🔇 Additional comments (1)
src/standalone/Bucket/Bucket.jl (1)
324-326: Aux cache extension looks correct.Thanks for appending
total_energyandtotal_watertoauxiliary_types; the tuple order stays consistent with the correspondingauxiliary_varsentry, so the cache will initialise properly.
juliasloan25
force-pushed
the
tr/add-energy_per_area-bucket
branch
2 times, most recently
from
ea5237d to
855d703
Compare
juliasloan25
changed the title
juliasloan25
force-pushed
the
tr/add-energy_per_area-bucket
branch
from
855d703 to
6edfb00
Compare
src/standalone/Bucket/Bucket.jl
Outdated
| t, | ||
| ) | ||
| ρ_cloud_liq = LP.ρ_cloud_liq(model.parameters.earth_param_set) | ||
| @. surface_field = (Y.bucket.σS + Y.bucket.W + Y.bucket.Ws) * ρ_cloud_liq |
There was a problem hiding this comment.
This is converting from a liquid volume per area to a mass per area. I think given the function name this should be handled in the coupler...
juliasloan25
force-pushed
the
tr/add-energy_per_area-bucket
branch
from
6edfb00 to
8629643
Compare
Purpose
closes #1091
Content
e_per_areatotal_energy_per_area!for the BucketModele_per_areausingtotal_energy_per_area!inupdate_aux!