Testing updated RRTMGPInterface following RRTMGP changes to deep-atmosphere metrics #3779
Conversation
akshaysridhar
force-pushed
the
as/test-rad-update
branch
2 times, most recently
from
0e11ab6 to
670131b
Compare
akshaysridhar
force-pushed
the
as/test-rad-update
branch
3 times, most recently
from
f199d7f to
636b176
Compare
akshaysridhar
force-pushed
the
as/test-rad-update
branch
3 times, most recently
from
9f7657d to
bc74fda
Compare
|
cc @sriharshakandala This is rebased and ready for review. The entire interface will be revamped shortly to match the new API changes in RRTMGP in future PRs. |
WalkthroughThis update enables deep atmosphere modeling by adding a Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (12)
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: 1
🔭 Outside diff range comments (1)
src/parameterized_tendencies/radiation/RRTMGPInterface.jl (1)
19-45:⚠️ Potential issueNew
deep_atmospherefield breaks zero-arg constructorsAll four radiation-mode structs now require an extra
deep_atmosphere::Bool, but the file still passes around instances created with zero arguments (GrayRadiation(),ClearSkyRadiation(), …).
Unless companion outer constructors were updated elsewhere, any call site that still doesClearSkyRadiation()will now throw aMethodError.Add default constructors or make the new flag a keyword with a sensible default (
false) to keep downstream code working.
♻️ Duplicate comments (1)
src/parameterized_tendencies/radiation/RRTMGPInterface.jl (1)
913-920: Same symbol/string mismatch here – see previous comment.
Fixing the guard tohaskey(dict, :planet_radius)(and adding the early error) will make this block execute.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
.buildkite/Manifest-v1.11.toml(5 hunks)config/longrun_configs/amip_target_edonly.yml(1 hunks)src/parameterized_tendencies/radiation/RRTMGPInterface.jl(16 hunks)src/parameterized_tendencies/radiation/radiation.jl(4 hunks)src/solver/model_getters.jl(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: ci 1.11 - windows-latest
- GitHub Check: downstream ClimaCoupler.jl (1.11)
- GitHub Check: test (1.11)
- GitHub Check: downstream ClimaCoupler.jl (1.10)
🔇 Additional comments (12)
.buildkite/Manifest-v1.11.toml (5)
580-582: Package upgrade for CubedSphereThe CubedSphere package was updated from version 0.3.1 to 0.3.2, likely to support the deep atmosphere modeling functionality.
731-734: ExactPredicates dependency updateExactPredicates package was updated to version 2.2.8 and now depends on
StaticArraysinstead ofStaticArraysCore.
1158-1171: IntervalArithmetic dependency restructuringIntervalArithmetic has undergone significant restructuring:
- Removed several dependencies (FastRounding, LinearAlgebra, etc.)
- Added new dependencies (MacroTools, OpenBLASConsistentFPCSR_jll)
- Added weak dependencies and extension mappings
This update supports the numerical precision needed for deep atmosphere calculations.
1764-1768: New dependency addedAdded OpenBLASConsistentFPCSR_jll package, which provides consistent floating-point control status registers for OpenBLAS operations. This supports more stable numerical computations required for deep atmosphere modeling.
2467-2469: TaylorSeries version updateTaylorSeries package updated from 0.18.5 to 0.19.1, providing improved polynomial approximation capabilities needed for deep atmosphere calculations.
config/longrun_configs/amip_target_edonly.yml (1)
5-6: Enable deep atmosphere modeAdded two important configuration parameters:
deep_atmosphere: true- Enables deep atmosphere modeling which corrects for metric scalingmesh_warp_type: "SLEVE"- Specifies smooth level vertical (SLEVE) coordinate systemThese changes are central to the purpose of this PR which tests updated RRTMGP interface for deep atmospheres.
src/parameterized_tendencies/radiation/radiation.jl (3)
109-111: Extract planet radius from spherical geometryAdded conditional logic to extract planet radius from the grid's global geometry when it's a spherical geometry. This parameter is crucial for deep atmosphere modeling.
123-134: Conditionally include planet radius in model parametersAdded conditional logic for the
zkwargsnamed tuple to include the planet radius only when using spherical geometry. This ensures correct metric scaling for deep atmospheres while maintaining compatibility with other geometry types.
275-289: Consistent conditional planet radius handlingImplemented the same conditional planet radius logic in another overload of the function, maintaining consistency throughout the radiation model.
src/solver/model_getters.jl (2)
241-241: Extract deep_atmosphere parameter from configAdded code to extract the
deep_atmosphereparameter from the parsed arguments. This is needed to pass the configuration setting to the radiation model constructors.
261-261: Propagate deep_atmosphere parameter to radiation modesUpdated all radiation mode constructors to include the
deep_atmosphereparameter, ensuring the setting is properly propagated to:
- GrayRadiation
- ClearSkyRadiation
- AllSkyRadiation
- AllSkyRadiationWithClearSkyDiagnostics
This completes the connection between the configuration file and the radiation model implementation.
Also applies to: 263-268, 270-278, 280-288
src/parameterized_tendencies/radiation/RRTMGPInterface.jl (1)
1164-1182: Verify solver signatures after the newmetric_scalingargument
solve_lw!/solve_sw!are now invoked with one extra positional parameter.
Please ensure every corresponding method inRRTMGP.RTESolverwas updated accordingly; otherwise compilation will fail or, worse, the wrong method will be dispatched.Also applies to: 1203-1252
akshaysridhar
force-pushed
the
as/test-rad-update
branch
from
b4e8fcd to
720d69e
Compare
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 (6)
.buildkite/Manifest-v1.11.toml(5 hunks)config/longrun_configs/amip_target_edonly.yml(1 hunks)perf/jet_test_nfailures.jl(1 hunks)src/parameterized_tendencies/radiation/RRTMGPInterface.jl(16 hunks)src/parameterized_tendencies/radiation/radiation.jl(4 hunks)src/solver/model_getters.jl(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- config/longrun_configs/amip_target_edonly.yml
- src/solver/model_getters.jl
- src/parameterized_tendencies/radiation/radiation.jl
- .buildkite/Manifest-v1.11.toml
- src/parameterized_tendencies/radiation/RRTMGPInterface.jl
⏰ Context from checks skipped due to timeout of 90000ms (9)
- 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.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: evaluate
akshaysridhar
force-pushed
the
as/test-rad-update
branch
2 times, most recently
from
1670d69 to
38fe2ca
Compare
modified: src/parameterized_tendencies/radiation/RRTMGPInterface.jl modified: src/parameterized_tendencies/radiation/radiation.jl modified: src/solver/model_getters.jl modified: .buildkite/Manifest-v1.11.toml modified: perf/jet_test_nfailures.jl
akshaysridhar
force-pushed
the
as/test-rad-update
branch
from
38fe2ca to
24f6d36
Compare
|
This looks good to me overall. |
|
We might need to reset |
Indeed, this is behaviour changing for some spherical cases. I'm running a coupler 30 elem |
akshaysridhar
force-pushed
the
as/test-rad-update
branch
from
91cacfb to
24f6d36
Compare
|
@sriharshakandala https://buildkite.com/clima/climacoupler-coarse-nightly-amip/builds/317/steps Here is the nightly build that exercises this. (The 30 elem case is still running) |
| if ᶜspace.grid.global_geometry isa | ||
| Geometry.AbstractSphericalGlobalGeometry | ||
| kwargs = (; | ||
| kwargs..., | ||
| center_z = Fields.field2array(ᶜz), | ||
| face_z = Fields.field2array(ᶠz), | ||
| planet_radius = planet_radius, | ||
| ) | ||
| else | ||
| kwargs = (; | ||
| kwargs..., | ||
| center_z = Fields.field2array(ᶜz), | ||
| face_z = Fields.field2array(ᶠz), | ||
| ) | ||
| end |
There was a problem hiding this comment.
| if ᶜspace.grid.global_geometry isa | |
| Geometry.AbstractSphericalGlobalGeometry | |
| kwargs = (; | |
| kwargs..., | |
| center_z = Fields.field2array(ᶜz), | |
| face_z = Fields.field2array(ᶠz), | |
| planet_radius = planet_radius, | |
| ) | |
| else | |
| kwargs = (; | |
| kwargs..., | |
| center_z = Fields.field2array(ᶜz), | |
| face_z = Fields.field2array(ᶠz), | |
| ) | |
| end | |
| kwargs = (; | |
| kwargs..., | |
| center_z = Fields.field2array(ᶜz), | |
| face_z = Fields.field2array(ᶠz), | |
| ) | |
| if ᶜspace.grid.global_geometry isa | |
| Geometry.AbstractSphericalGlobalGeometry | |
| planet_radius = ᶜspace.grid.global_geometry.radius | |
| kwargs = (; kwargs..., planet_radius) | |
| end |
This could be a little less repetitive, and it would be clearer if you extracted planet_radius here instead of the beginning of the function. Same for the GrayRadiation method above.
Updated RRTMGPInterface to account for
metric scalingcorrection fordeep atmospheres