independent exchange space #1292
Conversation
juliasloan25
changed the title
WalkthroughThis change decouples the coupler boundary space, land surface space, and atmosphere space, allowing them to be independent grids. It introduces new constructors and logic for creating land domains, removes the requirement to pass the boundary space to land and ocean model constructors, and adds options for sharing or not sharing the surface space via configuration and CLI. Field updates throughout surface and ocean models are refactored to use spatial remapping ( Assessment against linked issues
Possibly related PRs
Note ⚡️ AI Code Reviews for VS Code, Cursor, WindsurfCodeRabbit now has a plugin for VS Code, Cursor and Windsurf. This brings AI code reviews directly in the code editor. Each commit is reviewed immediately, finding bugs before the PR is raised. Seamless context handoff to your AI code agent ensures that you can easily incorporate review feedback. Note ⚡️ Faster reviews with cachingCodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure 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: 3
🧹 Nitpick comments (1)
src/Utilities.jl (1)
196-226: Add early–return fast‑path & cut allocationsNice to see proper interpolation in place 🎉.
Two low‑effort tweaks will avoid a lot of needless work:
- Skip all processing when source and target spaces coincide.
- Re‑use the existing coordinate fields instead of
copy → field2array, which doubles memory traffic.function bilinear_remap(source_field, target_space) - # Get vector of LatLongPoints for the target space to get the hcoords - field_lat = copy(CC.Fields.coordinate_field(target_space).lat) - arr_lat = CC.Fields.field2array(field_lat) - field_long = copy(CC.Fields.coordinate_field(target_space).long) - arr_long = CC.Fields.field2array(field_long) - hcoords = CC.Geometry.LatLongPoint.(arr_lat, arr_long) + # Fast path: the spaces already match – avoid remap/allocations + axes(source_field) === target_space && return source_field + + # Build LatLongPoint vector without extra copies + lat_vals = CC.Fields.field2array(CC.Fields.coordinate_field(target_space).lat) + long_vals = CC.Fields.field2array(CC.Fields.coordinate_field(target_space).long) + hcoords = CC.Geometry.LatLongPoint.(lat_vals, long_vals)This preserves behaviour while cutting allocations roughly in half and makes the “same–space” case O(1).
(No public API change.)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
docs/src/fieldexchanger.md(0 hunks)experiments/ClimaEarth/components/land/climaland_bucket.jl(4 hunks)experiments/ClimaEarth/components/land/climaland_helpers.jl(1 hunks)experiments/ClimaEarth/components/land/climaland_integrated.jl(1 hunks)experiments/ClimaEarth/components/ocean/prescr_ocean.jl(3 hunks)experiments/ClimaEarth/components/ocean/prescr_seaice.jl(5 hunks)experiments/ClimaEarth/components/ocean/slab_ocean.jl(4 hunks)experiments/ClimaEarth/setup_run.jl(4 hunks)src/FieldExchanger.jl(2 hunks)src/FluxCalculator.jl(1 hunks)src/Utilities.jl(1 hunks)test/field_exchanger_tests.jl(0 hunks)
💤 Files with no reviewable changes (2)
- docs/src/fieldexchanger.md
- test/field_exchanger_tests.jl
🔇 Additional comments (15)
experiments/ClimaEarth/components/ocean/prescr_ocean.jl (1)
36-37: Consistent variable renaming fromspacetoboundary_spaceThe renaming from
spacetoboundary_spaceimproves the code clarity by explicitly indicating that this represents the exchange grid or boundary between components. This aligns with similar changes in other ocean component files.Also applies to: 53-54, 79-80, 84-85, 90-91, 94-96
experiments/ClimaEarth/components/ocean/prescr_seaice.jl (1)
52-54: Consistent variable renaming fromspacetoboundary_spaceThe renaming improves clarity and maintains consistency with other ocean component files. No functional changes introduced.
Also applies to: 84-85, 105-106, 111-112, 120-121, 122-126
src/FieldExchanger.jl (1)
83-84: Replaced dummy remapping with proper bilinear interpolationThe direct field copying via
dummmy_remap!has been replaced with properbilinear_remap!throughout the file. This is a significant improvement that allows proper interpolation between different spatial domains.Also applies to: 86-87, 89-91, 111-112, 114-115, 117-118
experiments/ClimaEarth/components/land/climaland_integrated.jl (1)
455-463: Updated atmospheric field remapping to use bilinear interpolationAll atmospheric fields are now properly remapped using bilinear interpolation instead of direct copying. This change aligns with the broader PR objective to make the exchange grid independent and implement meaningful remapping.
experiments/ClimaEarth/components/land/climaland_helpers.jl (1)
30-35: Function signature refactored to decouple from atmospheric space.The function now takes explicit element counts instead of deriving them from the atmosphere boundary space, supporting grid independence.
src/FluxCalculator.jl (3)
297-302: Correctly implemented bilinear remapping for atmospheric variables.The code now properly remaps height and wind components to the boundary space.
303-303: Thermodynamic state not yet remapped.A TODO comment indicates future work to remap thermodynamic state components.
Consider implementing the remapping now to maintain consistency with other fields.
309-309: Surface thermodynamic state calculation simplified.Now using coupler surface field directly instead of atmosphere interface state.
experiments/ClimaEarth/setup_run.jl (4)
214-215: Good reminder about ETOPO direct access improvement.Obtaining elevation data directly would further reduce dependencies.
234-239: Independent boundary space construction.Explicit creation of CubedSphereSpace instead of reusing atmosphere space supports the exchange grid independence goal.
315-315: Consistent boundary_space naming.Changed positional to named parameter for clarity and consistency.
418-419: Fixed assertion check for energy conservation.Now correctly verifies comms context rather than boundary space.
experiments/ClimaEarth/components/ocean/slab_ocean.jl (3)
36-43: Standardized parameter naming to boundary_space.Ensures consistency with other ocean/coupler components.
52-53: Simplified function return value.Now returns only the prognostic variable instead of a tuple.
75-88: Updated initialization with boundary_space naming.All references correctly use the renamed parameter throughout the function.
juliasloan25
force-pushed
the
js/exchange-space
branch
from
4470462 to
cdd60f8
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
experiments/ClimaEarth/components/land/climaland_bucket.jl (1)
70-88:⚠️ Potential issueUser-supplied
nelementsis ignoredImmediately after accepting
nelementsas an argument, the code overrides it:nelements = (50, 10)This drops caller intent and makes the keyword meaningless.
- nelements = (50, 10) + # keep the caller-supplied valueDelete the reassignment or rename the local variable.
🧹 Nitpick comments (1)
experiments/ClimaEarth/components/land/climaland_helpers.jl (1)
22-35: Docstring & signature driftThe docstring still advertises
make_land_domain(nelements, depth)while the actual method ismake_land_domain(depth; nelements=…). This mismatch will confuse users (and IDE tooling).-""" - make_land_domain(nelements::Tuple{Int, Int}, depth::FT) where {FT} +""" + make_land_domain(depth::FT; nelements = (101, 15), dz_tuple = (10, 0.05))Synchronise the two and mention default keyword values.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
experiments/ClimaEarth/components/land/climaland_bucket.jl(4 hunks)experiments/ClimaEarth/components/land/climaland_helpers.jl(1 hunks)experiments/ClimaEarth/components/land/climaland_integrated.jl(1 hunks)experiments/ClimaEarth/components/ocean/prescr_ocean.jl(3 hunks)experiments/ClimaEarth/components/ocean/prescr_seaice.jl(5 hunks)experiments/ClimaEarth/components/ocean/slab_ocean.jl(5 hunks)experiments/ClimaEarth/setup_run.jl(4 hunks)src/Interfacer.jl(1 hunks)src/Utilities.jl(0 hunks)
💤 Files with no reviewable changes (1)
- src/Utilities.jl
✅ Files skipped from review due to trivial changes (3)
- experiments/ClimaEarth/components/land/climaland_integrated.jl
- experiments/ClimaEarth/components/ocean/prescr_ocean.jl
- experiments/ClimaEarth/components/ocean/prescr_seaice.jl
🚧 Files skipped from review as they are similar to previous changes (2)
- experiments/ClimaEarth/setup_run.jl
- experiments/ClimaEarth/components/ocean/slab_ocean.jl
🧰 Additional context used
🧠 Learnings (1)
experiments/ClimaEarth/components/land/climaland_bucket.jl (1)
Learnt from: juliasloan25
PR: CliMA/ClimaCoupler.jl#1309
File: docs/src/fieldexchanger.md:14-16
Timestamp: 2025-04-29T20:24:04.124Z
Learning: The turbulent fluxes (F_lh and F_sh, formerly F_turb_energy) are calculated within the coupler by the FluxCalculator module, not imported from the atmosphere via import_atmos_fields!.
🔇 Additional comments (1)
experiments/ClimaEarth/components/land/climaland_bucket.jl (1)
95-100: Surface-elevation fallback OKNice touch defaulting to a zero field and remapping when provided.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
experiments/ClimaEarth/components/land/climaland_bucket.jl (1)
332-337: Align caller with callee to avoid crashFollowing on the previous comment, updating the caller is the minimal fix:
- Interfacer.update_field!(sim, Val(:turbulent_energy_flux), (; F_lh, F_sh)) + Interfacer.update_field!(sim, Val(:turbulent_energy_flux), (; lhf = F_lh, shf = F_sh))
🧹 Nitpick comments (4)
experiments/ClimaEarth/components/land/climaland_integrated.jl (3)
61-64:boundary_spaceparameter is now unusedThe constructor still takes
boundary_spacebut nothing in the body references it, which may puzzle
users and static analysers.Consider deleting the argument or prefixing with
_to make the intent explicit.
70-73: Hard-codingnelementsloses flexibilitySimilar to the bucket case, the resolution is fixed here. Expose it as a keyword or reuse an
existing parameter so that tests can down-sample and production runs up-sample without code edits.
550-551: Unused localland_space
land_spaceis computed but never referenced afterwards – dead code that can be dropped.experiments/ClimaEarth/components/atmosphere/climaatmos.jl (1)
354-360: Superfluous variable & missed remap
atmos_surface_spaceis computed but not used in this branch; meanwhile the remap targets the
scratch field without specifying the surface space. Drop the unused variable or hand it to
Interfacer.remap!for clarity.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
docs/src/utilities.md(0 hunks)experiments/ClimaEarth/components/atmosphere/climaatmos.jl(4 hunks)experiments/ClimaEarth/components/land/climaland_bucket.jl(4 hunks)experiments/ClimaEarth/components/land/climaland_helpers.jl(1 hunks)experiments/ClimaEarth/components/land/climaland_integrated.jl(7 hunks)experiments/ClimaEarth/components/ocean/prescr_seaice.jl(2 hunks)experiments/ClimaEarth/components/ocean/slab_ocean.jl(4 hunks)experiments/ClimaEarth/setup_run.jl(3 hunks)src/ConservationChecker.jl(1 hunks)src/Interfacer.jl(1 hunks)src/Utilities.jl(1 hunks)src/surface_stub.jl(1 hunks)test/utilities_tests.jl(0 hunks)
💤 Files with no reviewable changes (2)
- docs/src/utilities.md
- test/utilities_tests.jl
✅ Files skipped from review due to trivial changes (1)
- src/Utilities.jl
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Interfacer.jl
- experiments/ClimaEarth/setup_run.jl
- experiments/ClimaEarth/components/ocean/slab_ocean.jl
🧰 Additional context used
🧠 Learnings (3)
experiments/ClimaEarth/components/ocean/prescr_seaice.jl (1)
Learnt from: juliasloan25
PR: CliMA/ClimaCoupler.jl#1309
File: docs/src/fieldexchanger.md:14-16
Timestamp: 2025-04-29T20:24:04.124Z
Learning: The turbulent fluxes (F_lh and F_sh, formerly F_turb_energy) are calculated within the coupler by the FluxCalculator module, not imported from the atmosphere via import_atmos_fields!.
experiments/ClimaEarth/components/land/climaland_bucket.jl (1)
Learnt from: juliasloan25
PR: CliMA/ClimaCoupler.jl#1309
File: docs/src/fieldexchanger.md:14-16
Timestamp: 2025-04-29T20:24:04.124Z
Learning: The turbulent fluxes (F_lh and F_sh, formerly F_turb_energy) are calculated within the coupler by the FluxCalculator module, not imported from the atmosphere via import_atmos_fields!.
experiments/ClimaEarth/components/atmosphere/climaatmos.jl (1)
Learnt from: juliasloan25
PR: CliMA/ClimaCoupler.jl#1309
File: docs/src/fieldexchanger.md:14-16
Timestamp: 2025-04-29T20:24:04.124Z
Learning: The turbulent fluxes (F_lh and F_sh, formerly F_turb_energy) are calculated within the coupler by the FluxCalculator module, not imported from the atmosphere via import_atmos_fields!.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: ci 1.11 - windows-latest
- GitHub Check: ci 1.11 - ubuntu-latest
- GitHub Check: ci 1.10 - windows-latest
- GitHub Check: test (1.11)
- GitHub Check: ci 1.10 - ubuntu-latest
- GitHub Check: test (1.10)
- GitHub Check: docbuild
🔇 Additional comments (5)
experiments/ClimaEarth/components/ocean/prescr_seaice.jl (2)
166-176: Direct assignments refactored to use Interfacer.remap!Standardizing field updates through remapping instead of direct assignments. This enables proper interpolation between potentially different spatial grids.
195-196: Added explicit return nothingGood practice for clarity in mutating functions.
src/ConservationChecker.jl (1)
157-157: Replaced swap_space! with Interfacer.remapThis change aligns with the PR goal of providing proper remapping between different spatial discretizations, replacing the removed swap_space! utility.
src/surface_stub.jl (1)
51-64: Direct field assignments replaced with Interfacer.remap!Consistent with other component updates, standardizing all field updates through the remapping interface rather than direct assignments.
experiments/ClimaEarth/components/land/climaland_helpers.jl (1)
22-36: Land domain creation decoupled from atmosphere spaceGood refactoring to make land space independent of atmosphere space. Function now creates domains with explicit parameters rather than deriving from atmosphere boundary space.
juliasloan25
force-pushed
the
js/exchange-space
branch
2 times, most recently
from
744d67f to
bfa4bbe
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
experiments/ClimaEarth/components/land/climaland_bucket.jl (1)
310-311: TODO comment needs resolutionThe code still contains a TODO about accounting for sublimation in the turbulent moisture flux.
Do you want me to suggest an implementation approach for handling sublimation in this context?
experiments/ClimaEarth/components/atmosphere/climaatmos.jl (2)
313-316: Allocation concern in temperature field remappingThe code correctly remaps surface fields but notes an allocation issue that could be optimized.
Consider adding two additional scratch fields to avoid the allocations when remapping temperature and humidity.
396-399: Another allocation in turbulent flux remappingSimilar allocation issue when remapping momentum flux components.
Add one more scratch field to avoid the allocation of
F_turb_ρτyz_atmos.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
experiments/ClimaEarth/components/atmosphere/climaatmos.jl(5 hunks)experiments/ClimaEarth/components/land/climaland_bucket.jl(5 hunks)experiments/ClimaEarth/components/land/climaland_integrated.jl(8 hunks)experiments/ClimaEarth/setup_run.jl(3 hunks)experiments/ClimaEarth/user_io/debug_plots.jl(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- experiments/ClimaEarth/user_io/debug_plots.jl
🚧 Files skipped from review as they are similar to previous changes (2)
- experiments/ClimaEarth/setup_run.jl
- experiments/ClimaEarth/components/land/climaland_integrated.jl
🧰 Additional context used
🧠 Learnings (2)
experiments/ClimaEarth/components/land/climaland_bucket.jl (1)
Learnt from: juliasloan25
PR: CliMA/ClimaCoupler.jl#1309
File: docs/src/fieldexchanger.md:14-16
Timestamp: 2025-04-29T20:24:04.124Z
Learning: The turbulent fluxes (F_lh and F_sh, formerly F_turb_energy) are calculated within the coupler by the FluxCalculator module, not imported from the atmosphere via import_atmos_fields!.
experiments/ClimaEarth/components/atmosphere/climaatmos.jl (1)
Learnt from: juliasloan25
PR: CliMA/ClimaCoupler.jl#1309
File: docs/src/fieldexchanger.md:14-16
Timestamp: 2025-04-29T20:24:04.124Z
Learning: The turbulent fluxes (F_lh and F_sh, formerly F_turb_energy) are calculated within the coupler by the FluxCalculator module, not imported from the atmosphere via import_atmos_fields!.
⏰ 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: ci 1.10 - ubuntu-latest
- GitHub Check: test (1.11)
- GitHub Check: test (1.10)
- GitHub Check: docbuild
🔇 Additional comments (5)
experiments/ClimaEarth/components/land/climaland_bucket.jl (3)
70-72: Good defaults for land domain configurationDefault values for
nelements,depth, anddz_tuplelook appropriate for typical land simulations.
90-95: Smart elevation handling with remappingClean implementation that either creates a zero elevation field or remaps provided elevation to the land surface space.
300-303: Fixed field name mismatch in turbulent flux handlingThe implementation now correctly uses
F_lhandF_shfields, resolving the previous field name mismatch issue.experiments/ClimaEarth/components/atmosphere/climaatmos.jl (2)
116-124: Useful utility for getting atmosphere surface spaceGood addition that centralizes the logic for obtaining the atmosphere's surface space.
638-646: Proper wind speed remapping for albedo calculationThe albedo calculation now correctly remaps wind speed to the target field axes.
juliasloan25
force-pushed
the
js/exchange-space
branch
from
ef96b2c to
9be65d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
experiments/ClimaEarth/components/land/climaland_bucket.jl (1)
29-29: Update docstring to match implementationThe docstring still refers to "boundary space" though the implementation no longer uses it.
-- `area_fraction::A`: A ClimaCore Field on the boundary space representing the surface area fraction of this component model. +- `area_fraction::A`: A ClimaCore Field representing the surface area fraction of this component model.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
config/ci_configs/amip_albedo_function.yml(1 hunks)config/ci_configs/slabplanet_albedo_function.yml(1 hunks)experiments/ClimaEarth/cli_options.jl(1 hunks)experiments/ClimaEarth/components/land/climaland_bucket.jl(5 hunks)experiments/ClimaEarth/components/land/climaland_helpers.jl(1 hunks)experiments/ClimaEarth/components/land/climaland_integrated.jl(8 hunks)experiments/ClimaEarth/jl_QsN1FG/four_steps/output_0000/clima_atmos/.yml(1 hunks)experiments/ClimaEarth/jl_QsN1FG/four_steps/output_0000/clima_atmos/_parameters.toml(1 hunks)experiments/ClimaEarth/jl_QsN1FG/four_steps/output_active(1 hunks)experiments/ClimaEarth/jl_hptaEv/four_steps/output_0000/clima_atmos/.yml(1 hunks)experiments/ClimaEarth/jl_hptaEv/four_steps/output_0000/clima_atmos/_parameters.toml(1 hunks)experiments/ClimaEarth/jl_hptaEv/four_steps/output_active(1 hunks)experiments/ClimaEarth/setup_run.jl(5 hunks)experiments/ClimaEarth/user_io/arg_parsing.jl(2 hunks)src/Interfacer.jl(1 hunks)
✅ Files skipped from review due to trivial changes (8)
- experiments/ClimaEarth/jl_QsN1FG/four_steps/output_active
- experiments/ClimaEarth/jl_hptaEv/four_steps/output_active
- config/ci_configs/slabplanet_albedo_function.yml
- config/ci_configs/amip_albedo_function.yml
- experiments/ClimaEarth/jl_QsN1FG/four_steps/output_0000/clima_atmos/.yml
- experiments/ClimaEarth/jl_hptaEv/four_steps/output_0000/clima_atmos/_parameters.toml
- experiments/ClimaEarth/jl_QsN1FG/four_steps/output_0000/clima_atmos/_parameters.toml
- experiments/ClimaEarth/jl_hptaEv/four_steps/output_0000/clima_atmos/.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Interfacer.jl
- experiments/ClimaEarth/setup_run.jl
- experiments/ClimaEarth/components/land/climaland_integrated.jl
🧰 Additional context used
🧠 Learnings (1)
experiments/ClimaEarth/components/land/climaland_bucket.jl (1)
Learnt from: juliasloan25
PR: CliMA/ClimaCoupler.jl#1309
File: docs/src/fieldexchanger.md:14-16
Timestamp: 2025-04-29T20:24:04.124Z
Learning: The turbulent fluxes (F_lh and F_sh, formerly F_turb_energy) are calculated within the coupler by the FluxCalculator module, not imported from the atmosphere via import_atmos_fields!.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: ci 1.10 - windows-latest
- GitHub Check: ci 1.10 - ubuntu-latest
- GitHub Check: ci 1.11 - windows-latest
- GitHub Check: ci 1.11 - ubuntu-latest
🔇 Additional comments (14)
experiments/ClimaEarth/cli_options.jl (1)
85-89: New flag aligns with PR objectivesThe new
--share_surface_spaceflag enables decoupling the exchange grid as described in the PR objectives, with a sensible default value oftruefor backward compatibility.experiments/ClimaEarth/user_io/arg_parsing.jl (2)
91-92: Appropriate extraction of new space parameterClean implementation of the space sharing flag extraction.
133-133: Correctly propagates space parameter to simulation argsProperly exposes the
share_surface_spaceparameter through the named tuple for use in simulation setup.experiments/ClimaEarth/components/land/climaland_helpers.jl (2)
22-36: Good API design for simplified domain creationThis overload creates a lightweight domain without requiring boundary space, supporting the PR goal of independent land space.
38-75: Well-structured domain creation from shared surface spaceThe new overload elegantly handles creating a domain from shared surface space, aligning with the PR objective to specify exchange grid in the coupler.
This implementation preserves the polynomial degree from the shared surface space, ensuring consistent discretization across components.
experiments/ClimaEarth/components/land/climaland_bucket.jl (9)
70-74: Constructor signature supports independent land spaceConstructor now properly accepts domain parameters instead of relying on atmospheric boundary space.
86-93: Clean domain creation with shared space supportGood implementation of conditional domain creation based on shared surface space availability.
94-99: Flexible surface elevation handlingClear logic for either interpolating provided elevation or using zero elevation.
104-119: Albedo initializations use the right spaceAll albedo initialization methods correctly use surface_space.
295-296: Properly uses remapping for air densitySwitched to
Interfacer.remap!as required by PR objectives.
298-300: Correct unit conversion for liquid precipitationHandles liquid precipitation with proper density normalization.
302-307: Proper remapping for energy fluxesCorrectly implements remapping for radiative and turbulent energy fluxes.
309-315: Correct unit conversions with remappingProperly handles density normalization for snow and moisture fluxes.
335-337: Unified flux update approachUses higher-level update_field methods consistently for turbulent fluxes.
| end | ||
| function Interfacer.update_field!(sim::PrescribedIceSimulation, ::Val{:area_fraction}, field::CC.Fields.Field) | ||
| sim.integrator.p.area_fraction .= field | ||
| Interfacer.remap!(sim.integrator.p.area_fraction, field) |
There was a problem hiding this comment.
| Interfacer.remap!(sim.integrator.p.area_fraction, field) | |
| sim.integrator.p.area_fraction .= field |
juliasloan25
force-pushed
the
js/exchange-space
branch
2 times, most recently
from
f5e45f0 to
b49574c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
experiments/ClimaEarth/setup_run.jl (1)
228-239: Properly implemented independent exchange grid based on configurationThe code now either reuses the atmosphere horizontal space or creates a new independent surface space depending on the
share_surface_spaceparameter.One note: there's a TODO to get the radius from ClimaParams, which is not yet implemented.
Consider implementing the commented-out TODO for getting the radius from ClimaParams instead of hardcoding 6371e3.
experiments/ClimaEarth/components/atmosphere/climaatmos.jl (2)
306-320: Implemented remapping for surface temperature updatesReplaced direct field assignments with proper remapping to handle different grid spaces.
Note: There's a comment about this being allocating - future optimization opportunity.
Consider adding the two additional scratch fields mentioned in the comment to avoid allocations.
380-413: Implemented remapping for turbulent flux updatesComprehensive update to use remapping for all turbulent flux fields.
Similar to earlier methods, there's a comment about allocations that could be optimized.
Consider adding the mentioned additional scratch field to avoid allocations in the turbulent flux calculations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
config/ci_configs/amip_albedo_function.yml(1 hunks)config/ci_configs/slabplanet_albedo_function.yml(1 hunks)docs/src/interfacer.md(1 hunks)docs/src/utilities.md(0 hunks)experiments/ClimaEarth/cli_options.jl(1 hunks)experiments/ClimaEarth/components/atmosphere/climaatmos.jl(5 hunks)experiments/ClimaEarth/components/land/climaland_bucket.jl(6 hunks)experiments/ClimaEarth/components/land/climaland_helpers.jl(1 hunks)experiments/ClimaEarth/components/land/climaland_integrated.jl(8 hunks)experiments/ClimaEarth/components/ocean/prescr_seaice.jl(1 hunks)experiments/ClimaEarth/components/ocean/slab_ocean.jl(3 hunks)experiments/ClimaEarth/setup_run.jl(5 hunks)experiments/ClimaEarth/test/component_model_tests/climaland_tests.jl(2 hunks)experiments/ClimaEarth/user_io/arg_parsing.jl(2 hunks)experiments/ClimaEarth/user_io/debug_plots.jl(1 hunks)src/ConservationChecker.jl(3 hunks)src/FieldExchanger.jl(6 hunks)src/FluxCalculator.jl(1 hunks)src/Interfacer.jl(1 hunks)src/Utilities.jl(1 hunks)src/surface_stub.jl(1 hunks)test/utilities_tests.jl(0 hunks)
💤 Files with no reviewable changes (2)
- docs/src/utilities.md
- test/utilities_tests.jl
🚧 Files skipped from review as they are similar to previous changes (17)
- config/ci_configs/slabplanet_albedo_function.yml
- config/ci_configs/amip_albedo_function.yml
- docs/src/interfacer.md
- experiments/ClimaEarth/test/component_model_tests/climaland_tests.jl
- experiments/ClimaEarth/user_io/debug_plots.jl
- experiments/ClimaEarth/user_io/arg_parsing.jl
- experiments/ClimaEarth/cli_options.jl
- src/Utilities.jl
- src/FieldExchanger.jl
- src/surface_stub.jl
- src/FluxCalculator.jl
- experiments/ClimaEarth/components/ocean/prescr_seaice.jl
- src/ConservationChecker.jl
- src/Interfacer.jl
- experiments/ClimaEarth/components/ocean/slab_ocean.jl
- experiments/ClimaEarth/components/land/climaland_integrated.jl
- experiments/ClimaEarth/components/land/climaland_bucket.jl
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: ci 1.10 - windows-latest
- GitHub Check: ci 1.11 - windows-latest
- GitHub Check: ci 1.10 - ubuntu-latest
- GitHub Check: ci 1.11 - ubuntu-latest
- GitHub Check: ci 1.10 - macOS-latest
- GitHub Check: ci 1.11 - macOS-latest
🔇 Additional comments (13)
experiments/ClimaEarth/setup_run.jl (6)
110-120: Addedshare_surface_spaceparameter to CoupledSimulation argumentsThis new parameter implements the independent exchange space feature mentioned in the PR objectives.
275-277: Clear shared surface space determination logicThis logic properly implements the shared surface space mechanism.
278-294: Updated BucketSimulation constructor argumentsChanged from explicitly passing
boundary_spaceto passingshared_surface_space, which can be either the boundary space or nothing.
296-309: Updated ClimaLandSimulation constructor argumentsSimilarly updated to use the new
shared_surface_spaceparameter.
339-354: Removed boundary_space parameter from slabplanet land model constructorThe land model now handles its own space creation internally when no shared space is provided.
405-406: Updated assertion for energy conservation checksChanged to verify the comms context is a singleton instead of checking boundary space.
experiments/ClimaEarth/components/land/climaland_helpers.jl (2)
22-36: Simplified land domain creation without boundary spaceReimplemented
make_land_domainto create a domain without requiring a boundary space, using default parameters. This properly supports creating independent land surfaces.
38-87: Added new domain creation method using shared surface spaceImplemented a second overload that creates a land domain from a shared surface space. This enables either sharing or independence between atmosphere and land grids.
The implementation looks solid, handling coordinate fields, meshes, and spaces appropriately.
experiments/ClimaEarth/components/atmosphere/climaatmos.jl (5)
116-124: Added helper for getting atmosphere surface spaceThis utility function cleanly extracts the surface space for remapping operations.
356-361: Added remapping for emissivity updatesProperly handles remapping emissivity field onto the atmosphere space.
364-369: Added remapping for surface direct albedo updatesSimilar to emissivity, correctly implements remapping for albedo.
373-378: Added remapping for surface diffuse albedo updatesSame approach applied consistently to diffuse albedo.
636-644: Added remapping for water albedo calculationsUpdated to remap wind speed to the proper grid before calculating albedo.
juliasloan25
force-pushed
the
js/exchange-space
branch
2 times, most recently
from
1744476 to
fc7553e
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
experiments/ClimaEarth/components/atmosphere/climaatmos.jl (1)
378-398: 🛠️ Refactor suggestionEliminate allocation in turbulent flux remapping.
Similar to the temperature remapping, there's an allocation that could be avoided with an additional scratch field.
# Add a scratch field during initialization, then: - F_turb_ρτyz_atmos = Interfacer.remap(F_turb_ρτyz, atmos_surface_space) + Interfacer.remap!(sim.integrator.p.scratch.ᶠtemp_field_level2, F_turb_ρτyz) # F_turb_ρτyz_atmos
🧹 Nitpick comments (1)
experiments/ClimaEarth/components/land/climaland_integrated.jl (1)
58-91: Improved constructor with surface space independence.The constructor now properly creates its own land domain rather than requiring an external boundary space. The optional shared_surface_space parameter enables flexibility while maintaining independence.
I would suggest improving the parameter names documentation for clarity:
""" nelements::Tuple{Int, Int} = (101, 10) Tuple of (horizontal, vertical) element counts for the land domain depth::FT = FT(50) Depth of the soil column in meters dz_tuple::Tuple{FT, FT} = FT.((10.0, 0.05)) Tuple of (max_depth, min_depth) for vertical grid spacing shared_surface_space = nothing Optional surface space to share with other components """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
config/ci_configs/amip_albedo_function.yml(1 hunks)config/ci_configs/amip_coarse_mpi.yml(1 hunks)config/ci_configs/slabplanet_albedo_function.yml(1 hunks)docs/src/interfacer.md(1 hunks)docs/src/utilities.md(0 hunks)experiments/ClimaEarth/Manifest-v1.11.toml(1 hunks)experiments/ClimaEarth/Manifest.toml(1 hunks)experiments/ClimaEarth/cli_options.jl(1 hunks)experiments/ClimaEarth/components/atmosphere/climaatmos.jl(4 hunks)experiments/ClimaEarth/components/land/climaland_bucket.jl(6 hunks)experiments/ClimaEarth/components/land/climaland_helpers.jl(1 hunks)experiments/ClimaEarth/components/land/climaland_integrated.jl(8 hunks)experiments/ClimaEarth/components/ocean/prescr_seaice.jl(1 hunks)experiments/ClimaEarth/components/ocean/slab_ocean.jl(3 hunks)experiments/ClimaEarth/setup_run.jl(5 hunks)experiments/ClimaEarth/test/component_model_tests/climaland_tests.jl(2 hunks)experiments/ClimaEarth/user_io/arg_parsing.jl(2 hunks)experiments/ClimaEarth/user_io/debug_plots.jl(1 hunks)src/ConservationChecker.jl(3 hunks)src/FieldExchanger.jl(6 hunks)src/FluxCalculator.jl(1 hunks)src/Interfacer.jl(1 hunks)src/Utilities.jl(1 hunks)src/surface_stub.jl(1 hunks)test/utilities_tests.jl(0 hunks)
💤 Files with no reviewable changes (2)
- docs/src/utilities.md
- test/utilities_tests.jl
✅ Files skipped from review due to trivial changes (2)
- config/ci_configs/amip_coarse_mpi.yml
- experiments/ClimaEarth/Manifest.toml
🚧 Files skipped from review as they are similar to previous changes (16)
- config/ci_configs/amip_albedo_function.yml
- experiments/ClimaEarth/user_io/arg_parsing.jl
- experiments/ClimaEarth/test/component_model_tests/climaland_tests.jl
- config/ci_configs/slabplanet_albedo_function.yml
- experiments/ClimaEarth/user_io/debug_plots.jl
- src/FluxCalculator.jl
- docs/src/interfacer.md
- src/surface_stub.jl
- src/FieldExchanger.jl
- experiments/ClimaEarth/cli_options.jl
- src/Utilities.jl
- experiments/ClimaEarth/components/ocean/prescr_seaice.jl
- src/Interfacer.jl
- experiments/ClimaEarth/setup_run.jl
- src/ConservationChecker.jl
- experiments/ClimaEarth/components/ocean/slab_ocean.jl
🧰 Additional context used
🧠 Learnings (1)
experiments/ClimaEarth/components/land/climaland_bucket.jl (1)
Learnt from: juliasloan25
PR: CliMA/ClimaCoupler.jl#1309
File: docs/src/fieldexchanger.md:14-16
Timestamp: 2025-04-29T20:24:04.124Z
Learning: The turbulent fluxes (F_lh and F_sh, formerly F_turb_energy) are calculated within the coupler by the FluxCalculator module, not imported from the atmosphere via import_atmos_fields!.
⏰ Context from checks skipped due to timeout of 90000ms (7)
- 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
🔇 Additional comments (30)
experiments/ClimaEarth/Manifest-v1.11.toml (1)
398-400: Version bump for ClimaLand from 0.16.0 to 0.16.1The dependency update aligns with the PR objective of making the land space independent from the atmosphere space.
experiments/ClimaEarth/components/land/climaland_helpers.jl (2)
22-36: Simplified domain creation without requiring boundary spaceGood refactoring to allow creating a land domain using just scalar parameters without requiring a boundary space. This supports the PR's goal of decoupling land and atmosphere spaces.
38-87: New domain creation method using shared surface spaceThis new overload provides the alternative pathway for creating domains with shared surface space when desired, while maintaining the ability to create independent spaces. The vertical coordinate setup properly uses negative depths (-depth to 0).
experiments/ClimaEarth/components/land/climaland_bucket.jl (14)
66-70: Updated constructor parameters to support independent spacesThe constructor now supports both independent land spaces (with depth/nelements/dz_tuple) and shared spaces (with shared_surface_space), removing the boundary_space dependency.
72-72: Surface elevation made optionalMaking surface_elevation optional allows more flexibility and simplifies model initialization when topography isn't needed.
80-96: Improved domain initialization logicThe code now properly handles both creation pathways and ensures surface_elevation is either created or remapped to the correct space.
100-100: Updated albedo initialization to use surface_spaceReferences to boundary_space correctly replaced with surface_space.
104-105: Updated surface albedo initializationReferences to boundary_space correctly replaced with surface_space.
108-109: Updated albedo constructor parameterReferences to boundary_space correctly replaced with surface_space.
118-119: Updated prescribed albedo initializationReferences to boundary_space correctly replaced with surface_space.
295-296: Replaced direct assignment with remap for air densityUsing Interfacer.remap! ensures proper field interpolation between potentially different spatial discretizations.
298-300: Replaced direct assignment with remap for liquid precipitationUsing Interfacer.remap! ensures proper field interpolation while maintaining the density conversion.
302-303: Replaced direct assignment with remap for radiative energy fluxUsing Interfacer.remap! ensures proper field interpolation between potentially different spatial discretizations.
304-307: Updated turbulent energy flux handlingNow properly handles both latent and sensible heat fluxes with remapping to ensure correct spatial interpolation.
309-311: Replaced direct assignment with remap for snow precipitationUsing Interfacer.remap! ensures proper field interpolation while maintaining the density conversion.
313-315: Replaced direct assignment with remap for moisture fluxUsing Interfacer.remap! ensures proper field interpolation while maintaining the density conversion.
334-336: Updated flux calculator to use remappingProperly delegates to the update_field! methods which handle remapping instead of direct assignment.
experiments/ClimaEarth/components/atmosphere/climaatmos.jl (5)
114-121: Well-structured function for surface space extraction.The new
get_surface_spacefunction cleanly extracts the atmosphere's bottom level face space, which is critical for the independent exchange space architecture.
351-359: Good implementation of remapping for emissivity.The update correctly remaps the field to the atmosphere surface space before updating the model.
360-367: Consistent remapping approach for direct albedo.The implementation follows the same pattern as emissivity, maintaining consistency.
369-376: Matching implementation for diffuse albedo.The same pattern is correctly applied to the diffuse albedo field.
399-411: Clean implementation of vector flux updates.The code properly handles the tensor product of surface normal and remapped flux components.
experiments/ClimaEarth/components/land/climaland_integrated.jl (8)
25-25: Clarified documentation for area_fraction.The updated docstring correctly specifies that area_fraction is a ClimaCore Field on the boundary space.
127-127: Updated atmosphere coupling to use surface space.The change correctly updates the atmosphere coupling to use the land's surface space rather than an external boundary space.
414-416: Consistent use of remapping for diffuse fraction.Good replacement of direct assignment with proper remapping.
417-447: Updated all field updates to use remapping.All field update methods consistently use Interfacer.remap! instead of direct assignments, ensuring proper spatial interpolation.
560-578: Efficient use of scratch space for remapping.The flux calculation now properly uses scratch fields to avoid allocations during remapping of atmospheric state.
595-628: Well-structured flux combination with proper remapping.The code efficiently combines fluxes from different land components using remapping and temporary fields.
629-645: Consistent handling of momentum fluxes.The momentum flux remapping follows the same pattern as the energy fluxes.
646-654: Proper remapping of buoyancy flux.Good implementation of remapping for the buoyancy flux components.
| # Remap coupler fields onto the atmosphere surface space | ||
| atmos_surface_space = get_surface_space(sim) | ||
| temp_field_surface = sim.integrator.p.scratch.ᶠtemp_field_level | ||
| @assert axes(temp_field_surface) == atmos_surface_space | ||
|
|
||
| # NOTE: This is allocating! If we had 2 more scratch fields, we could avoid this | ||
| T_sfc_atmos = Interfacer.remap(csf.T_sfc, atmos_surface_space) | ||
| q_sfc_atmos = Interfacer.remap(csf.q_sfc, atmos_surface_space) | ||
| # Store `ρ_sfc_atmos` in an atmosphere scratch field on the surface space | ||
| temp_field_surface = | ||
| FluxCalculator.extrapolate_ρ_to_sfc.( | ||
| get_thermo_params(sim), | ||
| sim.integrator.p.precomputed.sfc_conditions.ts, | ||
| csf.T_sfc, | ||
| ) | ||
| T_sfc_atmos, | ||
| ) # ρ_sfc_atmos |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider allocating more scratch fields to avoid performance issues.
The remapping of T_sfc and q_sfc to the atmosphere space is currently allocating new memory. As noted in the comment, this could be avoided with additional scratch fields.
# Add these fields to the scratch space during initialization
# Then replace the allocating calls with in-place updates:
- T_sfc_atmos = Interfacer.remap(csf.T_sfc, atmos_surface_space)
- q_sfc_atmos = Interfacer.remap(csf.q_sfc, atmos_surface_space)
+ Interfacer.remap!(sim.integrator.p.scratch.ᶠtemp_field_level2, csf.T_sfc) # T_sfc_atmos
+ Interfacer.remap!(sim.integrator.p.scratch.ᶠtemp_field_level3, csf.q_sfc) # q_sfc_atmosCommittable suggestion skipped: line range outside the PR's diff.
| else | ||
| # Gather then broadcast the global hcoords and offsets | ||
| offset = [length(hcoords)] | ||
| all_hcoords = ClimaComms.bcast(comms_ctx, ClimaComms.gather(comms_ctx, hcoords)) | ||
| all_offsets = ClimaComms.bcast(comms_ctx, ClimaComms.gather(comms_ctx, offset)) | ||
|
|
||
| # Interpolate on root and broadcast to all processes | ||
| remapper = CC.Remapping.Remapper(source_space; target_hcoords = all_hcoords) | ||
| remapped_array = ClimaComms.bcast(comms_ctx, CC.Remapping.interpolate(remapper, field)) | ||
|
|
||
| my_ending_offset = sum(all_offsets[1:ClimaComms.mypid(comms_ctx)]) | ||
| my_starting_offset = my_ending_offset - offset[] | ||
|
|
||
| # Convert remapped array to a field in the target space on each process | ||
| return CC.Fields.array2field(remapped_array[(1 + my_starting_offset):my_ending_offset], target_space) |
There was a problem hiding this comment.
Could you add one test for this?
juliasloan25
force-pushed
the
js/exchange-space
branch
2 times, most recently
from
ab75024 to
a1d99d3
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
experiments/ClimaEarth/setup_run.jl (1)
349-364: 🛠️ Refactor suggestionSlabPlanet mode missing shared_surface_space parameter.
Unlike the AMIP/CMIP implementations which use the new shared_surface_space parameter, the SlabPlanet BucketSimulation constructor doesn't have this parameter. Consider adding it for consistency.
land_sim = BucketSimulation( FT; dt = component_dt_dict["dt_land"], tspan, start_date, output_dir = land_output_dir, area_fraction = land_fraction, saveat, + shared_surface_space, surface_elevation, land_temperature_anomaly, use_land_diagnostics, albedo_type = bucket_albedo_type, bucket_initial_condition, energy_check, parameter_files, )
🧹 Nitpick comments (3)
.buildkite/pipeline.yml (1)
80-83: Add dedicated MPI Interfacer test step
Good split of remapping tests into its own step. To avoid drift, consider using YAML anchors for the repeatedenv:andagents:blocks across both MPI steps.experiments/ClimaEarth/setup_run.jl (1)
226-228: Consider implementing the TODO for retrieving the radius parameter.The commented code refers to getting the planet radius from ClimaParams. This should be implemented to avoid hardcoding the radius value.
experiments/ClimaEarth/components/land/climaland_helpers.jl (1)
27-36: Consider making npolynomial configurable.The hardcoded npolynomial = 0 might limit flexibility. Consider making it a parameter with a default value.
function make_land_domain( depth::FT; nelements::Tuple{Int, Int} = (101, 15), dz_tuple::Tuple{FT, FT} = FT.((10.0, 0.05)), + npolynomial::Int = 0, ) where {FT} radius = FT(6378.1e3) - npolynomial = 0 domain = CL.Domains.SphericalShell(; radius, depth, nelements, npolynomial, dz_tuple) return domain end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
.buildkite/pipeline.yml(1 hunks)config/ci_configs/amip_albedo_function.yml(1 hunks)config/ci_configs/amip_coarse_mpi.yml(1 hunks)config/ci_configs/slabplanet_albedo_function.yml(1 hunks)docs/src/interfacer.md(1 hunks)docs/src/utilities.md(0 hunks)experiments/ClimaEarth/Manifest-v1.11.toml(1 hunks)experiments/ClimaEarth/Manifest.toml(1 hunks)experiments/ClimaEarth/cli_options.jl(1 hunks)experiments/ClimaEarth/components/atmosphere/climaatmos.jl(4 hunks)experiments/ClimaEarth/components/land/climaland_bucket.jl(6 hunks)experiments/ClimaEarth/components/land/climaland_helpers.jl(1 hunks)experiments/ClimaEarth/components/land/climaland_integrated.jl(8 hunks)experiments/ClimaEarth/components/ocean/prescr_seaice.jl(1 hunks)experiments/ClimaEarth/components/ocean/slab_ocean.jl(3 hunks)experiments/ClimaEarth/setup_run.jl(5 hunks)experiments/ClimaEarth/test/component_model_tests/climaland_tests.jl(2 hunks)experiments/ClimaEarth/user_io/arg_parsing.jl(2 hunks)experiments/ClimaEarth/user_io/debug_plots.jl(1 hunks)src/ConservationChecker.jl(3 hunks)src/FieldExchanger.jl(6 hunks)src/FluxCalculator.jl(1 hunks)src/Interfacer.jl(1 hunks)src/Utilities.jl(1 hunks)src/surface_stub.jl(1 hunks)test/interfacer_tests.jl(8 hunks)test/utilities_tests.jl(0 hunks)
💤 Files with no reviewable changes (2)
- docs/src/utilities.md
- test/utilities_tests.jl
✅ Files skipped from review due to trivial changes (2)
- experiments/ClimaEarth/test/component_model_tests/climaland_tests.jl
- src/Utilities.jl
🚧 Files skipped from review as they are similar to previous changes (20)
- config/ci_configs/slabplanet_albedo_function.yml
- docs/src/interfacer.md
- config/ci_configs/amip_albedo_function.yml
- config/ci_configs/amip_coarse_mpi.yml
- experiments/ClimaEarth/user_io/arg_parsing.jl
- experiments/ClimaEarth/user_io/debug_plots.jl
- experiments/ClimaEarth/cli_options.jl
- src/FieldExchanger.jl
- experiments/ClimaEarth/components/ocean/prescr_seaice.jl
- src/FluxCalculator.jl
- experiments/ClimaEarth/components/ocean/slab_ocean.jl
- test/interfacer_tests.jl
- experiments/ClimaEarth/Manifest-v1.11.toml
- experiments/ClimaEarth/Manifest.toml
- src/surface_stub.jl
- src/Interfacer.jl
- src/ConservationChecker.jl
- experiments/ClimaEarth/components/atmosphere/climaatmos.jl
- experiments/ClimaEarth/components/land/climaland_integrated.jl
- experiments/ClimaEarth/components/land/climaland_bucket.jl
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: ci 1.11 - windows-latest
- GitHub Check: ci 1.10 - windows-latest
- GitHub Check: ci 1.11 - ubuntu-latest
- GitHub Check: ci 1.10 - ubuntu-latest
🔇 Additional comments (4)
.buildkite/pipeline.yml (1)
73-78: Specify MPI utilities resources and context
Shortened timeout to 5 min and setCLIMACOMMS_CONTEXT="MPI"with 2 tasks and 16 GB memory. This matches the PR’s goal to isolate MPI utilities tests.experiments/ClimaEarth/setup_run.jl (2)
229-237: Good implementation of independent surface space creation.The conditional creation of boundary space effectively implements the PR objective of making the exchange grid independent from the atmosphere.
273-274: Clear implementation of surface space sharing logic.The shared_surface_space variable neatly handles the conditional surface space sharing.
experiments/ClimaEarth/components/land/climaland_helpers.jl (1)
48-71: Good implementation of shared surface space domain creation.The new overload effectively supports creating a land domain with a shared surface space, retrieving the polynomial degree from the input space.
juliasloan25
force-pushed
the
js/exchange-space
branch
from
a1d99d3 to
b8a4e5a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
experiments/ClimaEarth/components/atmosphere/climaatmos.jl (1)
309-311: 🛠️ Refactor suggestionOptimize by adding scratch fields to avoid allocations.
The remapping operations are currently allocating new memory unnecessarily.
- # NOTE: This is allocating! If we had 2 more scratch fields, we could avoid this - T_sfc_atmos = Interfacer.remap(csf.T_sfc, atmos_surface_space) - q_sfc_atmos = Interfacer.remap(csf.q_sfc, atmos_surface_space) + # Use additional scratch fields to avoid allocations + T_sfc_atmos = sim.integrator.p.scratch.ᶠtemp_field_level2 # Add this field during initialization + q_sfc_atmos = sim.integrator.p.scratch.ᶠtemp_field_level3 # Add this field during initialization + Interfacer.remap!(T_sfc_atmos, csf.T_sfc) + Interfacer.remap!(q_sfc_atmos, csf.q_sfc)
🧹 Nitpick comments (4)
experiments/ClimaEarth/setup_run.jl (1)
227-237: Consider getting radius from ClimaParams instead of hardcoding.The TODO comments about extracting radius from ClimaParams should be addressed for better maintainability.
- # TODO get radius from ClimaParams - # toml_dict = CP.create_toml_dict(FT, override_file=parameter_files[1]) - # (; radius) = CP.get_parameter_values(toml_dict, "planet_radius") - if share_surface_space - boundary_space = CC.Spaces.horizontal_space(atmos_sim.domain.face_space) - else - h_elem = config_dict["h_elem"] - n_quad_points = CC.Spaces.Quadratures.polynomial_degree( - CC.Spaces.quadrature_style(CC.Spaces.horizontal_space(atmos_sim.domain.face_space)), - ) - boundary_space = CC.CommonSpaces.CubedSphereSpace(FT; radius = FT(6371e3), n_quad_points, h_elem) - end + if share_surface_space + boundary_space = CC.Spaces.horizontal_space(atmos_sim.domain.face_space) + else + h_elem = config_dict["h_elem"] + n_quad_points = CC.Spaces.Quadratures.polynomial_degree( + CC.Spaces.quadrature_style(CC.Spaces.horizontal_space(atmos_sim.domain.face_space)), + ) + # Get radius from ClimaParams + toml_dict = CP.create_toml_dict(FT, override_file=parameter_files[1]) + (; radius) = CP.get_parameter_values(toml_dict, "planet_radius") + boundary_space = CC.CommonSpaces.CubedSphereSpace(FT; radius, n_quad_points, h_elem) + endexperiments/ClimaEarth/components/land/climaland_helpers.jl (1)
22-36: Consider aligning default parameters with climaland_bucket.jl.The default
nelementshere is(101, 15), but it's(50, 10)in climaland_bucket.jl. This inconsistency could cause unexpected behavior.- nelements::Tuple{Int, Int} = (101, 15), - dz_tuple::Tuple{FT, FT} = FT.((10.0, 0.05)), + nelements::Tuple{Int, Int} = (50, 10), + dz_tuple::Tuple{FT, FT} = FT.((1, 0.05)),experiments/ClimaEarth/components/land/climaland_bucket.jl (1)
313-315: Consider consolidating division by liquid water density.This division is repeated in multiple update methods, which could be inefficient.
- ρ_liq = LP.ρ_cloud_liq(sim.model.parameters.earth_param_set) - Interfacer.remap!(sim.integrator.p.bucket.turbulent_fluxes.vapor_flux, field ./ ρ_liq) # TODO: account for sublimation + # Divide by liquid water density once and then remap + ρ_liq = LP.ρ_cloud_liq(sim.model.parameters.earth_param_set) + normalized_field = field ./ ρ_liq + Interfacer.remap!(sim.integrator.p.bucket.turbulent_fluxes.vapor_flux, normalized_field) # TODO: account for sublimationexperiments/ClimaEarth/components/land/climaland_integrated.jl (1)
542-547: Documentation enhancement for flux calculationClear explanation of the current approach and its limitations. It would be good to create a follow-up issue to track the future improvement mentioned here.
Consider creating an issue to track the future work of computing fluxes directly on the boundary space.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (27)
.buildkite/pipeline.yml(1 hunks)config/ci_configs/amip_albedo_function.yml(1 hunks)config/ci_configs/amip_coarse_mpi.yml(1 hunks)config/ci_configs/slabplanet_albedo_function.yml(1 hunks)docs/src/interfacer.md(1 hunks)docs/src/utilities.md(0 hunks)experiments/ClimaEarth/Manifest-v1.11.toml(1 hunks)experiments/ClimaEarth/Manifest.toml(1 hunks)experiments/ClimaEarth/cli_options.jl(1 hunks)experiments/ClimaEarth/components/atmosphere/climaatmos.jl(4 hunks)experiments/ClimaEarth/components/land/climaland_bucket.jl(6 hunks)experiments/ClimaEarth/components/land/climaland_helpers.jl(1 hunks)experiments/ClimaEarth/components/land/climaland_integrated.jl(8 hunks)experiments/ClimaEarth/components/ocean/prescr_seaice.jl(1 hunks)experiments/ClimaEarth/components/ocean/slab_ocean.jl(4 hunks)experiments/ClimaEarth/setup_run.jl(5 hunks)experiments/ClimaEarth/test/component_model_tests/climaland_tests.jl(2 hunks)experiments/ClimaEarth/user_io/arg_parsing.jl(2 hunks)experiments/ClimaEarth/user_io/debug_plots.jl(1 hunks)src/ConservationChecker.jl(3 hunks)src/FieldExchanger.jl(6 hunks)src/FluxCalculator.jl(1 hunks)src/Interfacer.jl(1 hunks)src/Utilities.jl(1 hunks)src/surface_stub.jl(1 hunks)test/interfacer_tests.jl(8 hunks)test/utilities_tests.jl(0 hunks)
💤 Files with no reviewable changes (2)
- docs/src/utilities.md
- test/utilities_tests.jl
🚧 Files skipped from review as they are similar to previous changes (20)
- config/ci_configs/amip_coarse_mpi.yml
- config/ci_configs/slabplanet_albedo_function.yml
- config/ci_configs/amip_albedo_function.yml
- experiments/ClimaEarth/user_io/debug_plots.jl
- docs/src/interfacer.md
- experiments/ClimaEarth/cli_options.jl
- src/FieldExchanger.jl
- experiments/ClimaEarth/test/component_model_tests/climaland_tests.jl
- experiments/ClimaEarth/user_io/arg_parsing.jl
- src/Utilities.jl
- .buildkite/pipeline.yml
- experiments/ClimaEarth/components/ocean/prescr_seaice.jl
- src/FluxCalculator.jl
- src/surface_stub.jl
- experiments/ClimaEarth/Manifest.toml
- test/interfacer_tests.jl
- experiments/ClimaEarth/Manifest-v1.11.toml
- src/ConservationChecker.jl
- src/Interfacer.jl
- experiments/ClimaEarth/components/ocean/slab_ocean.jl
🧰 Additional context used
🧠 Learnings (2)
experiments/ClimaEarth/components/land/climaland_bucket.jl (1)
Learnt from: juliasloan25
PR: CliMA/ClimaCoupler.jl#1309
File: docs/src/fieldexchanger.md:14-16
Timestamp: 2025-04-29T20:24:04.124Z
Learning: The turbulent fluxes (F_lh and F_sh, formerly F_turb_energy) are calculated within the coupler by the FluxCalculator module, not imported from the atmosphere via import_atmos_fields!.
experiments/ClimaEarth/components/atmosphere/climaatmos.jl (1)
Learnt from: juliasloan25
PR: CliMA/ClimaCoupler.jl#1309
File: docs/src/fieldexchanger.md:14-16
Timestamp: 2025-04-29T20:24:04.124Z
Learning: The turbulent fluxes (F_lh and F_sh, formerly F_turb_energy) are calculated within the coupler by the FluxCalculator module, not imported from the atmosphere via import_atmos_fields!.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: ci 1.11 - windows-latest
- GitHub Check: ci 1.10 - ubuntu-latest
- GitHub Check: ci 1.11 - ubuntu-latest
- GitHub Check: ci 1.10 - windows-latest
🔇 Additional comments (18)
experiments/ClimaEarth/setup_run.jl (3)
115-115: Nice addition of theshare_surface_spaceparameter.This enables flexible coupling between atmosphere and land components.
274-275: Good approach to sharing surface space conditionally.Explicitly setting shared_surface_space based on the configuration parameter is clear and maintainable.
415-416: Well-structured assertion update for energy checks.The revised assertion correctly checks the communication context type.
experiments/ClimaEarth/components/land/climaland_helpers.jl (1)
38-87: Well-designed new function for creating land domain from shared surface space.This overload properly constructs a detailed domain with consistent vertical discretization and coordinate fields.
experiments/ClimaEarth/components/land/climaland_bucket.jl (5)
82-86: Good conditional domain creation based on shared_surface_space.The code appropriately handles both cases of shared and independent surface spaces.
91-95: Surface elevation handling improved with explicit remapping.The new code properly remaps surface elevation to the surface space or initializes it as zeros there.
295-296: LGTM: Clean replacement of assignment with remapping.Using
Interfacer.remap!ensures proper spatial interpolation between different discretizations.
304-307: Good restructuring of turbulent energy flux updates.Handling latent and sensible heat fluxes separately using named parameters improves code clarity.
334-336: Clean update to FluxCalculator.update_turbulent_fluxes!Using
Interfacer.update_field!with appropriate value tags ensures consistent remapping.experiments/ClimaEarth/components/atmosphere/climaatmos.jl (3)
120-122: Excellent addition of get_surface_space function.This helper function extracts the atmosphere surface space cleanly and can be reused across the codebase.
404-407: Good use of remapping for energy and moisture fluxes.This approach ensures consistent spatial handling between components.
408-411: Consistent handling of Monin-Obukhov parameters.Using
Interfacer.remap!for L_MO, ustar, and buoyancy_flux maintains consistency with other field updates.experiments/ClimaEarth/components/land/climaland_integrated.jl (6)
66-69: Added configuration parameters for land domain creationThese new parameters provide flexibility for configuring the land domain independently from the atmosphere, which aligns with the PR goal of making land space independent.
77-83: Improved domain creation logicThe code now correctly handles both independent and shared surface space scenarios, implementing a key requirement from the PR objectives.
87-92: Surface elevation handling with remappingGood addition of remapping for surface elevation when provided. This ensures proper interpolation between potentially different spatial discretizations.
414-448: Replaced direct field assignments with remappingAll field update methods now use
Interfacer.remap!instead of direct assignment, which properly handles the case where fields are defined on different spaces.
571-580: Optimized field remapping with scratch spaceGood use of scratch fields to avoid allocations during remapping. This helps mitigate potential performance issues from remapping operations.
603-612: Consistent remapping for flux combinationThe combined fluxes are now properly remapped from land space to boundary space, maintaining spatial consistency throughout the system.
Also applies to: 623-628, 635-645, 652-655
| # NOTE: This is allocating (F_turb_ρτyz_atmos)! If we had 1 more scratch field, we could avoid this | ||
| Interfacer.remap!(temp_field_surface, F_turb_ρτxz) # F_turb_ρτxz_atmos | ||
| F_turb_ρτyz_atmos = Interfacer.remap(F_turb_ρτyz, atmos_surface_space) # F_turb_ρτyz_atmos | ||
| sim.integrator.p.precomputed.sfc_conditions.ρ_flux_uₕ .= ( | ||
| surface_normal .⊗ | ||
| CA.C12.( | ||
| Utilities.swap_space!(axes(vec_ct12_ct1), F_turb_ρτxz) .* vec_ct12_ct1 .+ | ||
| Utilities.swap_space!(axes(vec_ct12_ct2), F_turb_ρτyz) .* vec_ct12_ct2, | ||
| surface_local_geometry, | ||
| ) | ||
| CA.C12.(temp_field_surface .* vec_ct12_ct1 .+ F_turb_ρτyz_atmos .* vec_ct12_ct2, surface_local_geometry) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid allocation in turbulent flux remapping.
Similar to the previous comment, additional scratch fields would improve performance.
- # NOTE: This is allocating (F_turb_ρτyz_atmos)! If we had 1 more scratch field, we could avoid this
- Interfacer.remap!(temp_field_surface, F_turb_ρτxz) # F_turb_ρτxz_atmos
- F_turb_ρτyz_atmos = Interfacer.remap(F_turb_ρτyz, atmos_surface_space) # F_turb_ρτyz_atmos
+ # Use scratch fields to avoid allocations
+ F_turb_ρτxz_atmos = temp_field_surface
+ F_turb_ρτyz_atmos = sim.integrator.p.scratch.ᶠtemp_field_level4 # Add this field during initialization
+ Interfacer.remap!(F_turb_ρτxz_atmos, F_turb_ρτxz)
+ Interfacer.remap!(F_turb_ρτyz_atmos, F_turb_ρτyz)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In experiments/ClimaEarth/components/atmosphere/climaatmos.jl around lines 392
to 397, the remapping of F_turb_ρτyz currently allocates a new array, which
impacts performance. To fix this, introduce an additional pre-allocated scratch
field for F_turb_ρτyz_atmos and use the in-place remap! function instead of
remap to avoid allocation during the turbulent flux remapping step.
| # The ClimaCore DataLayout underlying the remapped field uses | ||
| # Array instead of SubArray, so we can't compare the fields directly without Copy | ||
| @test field_source_space ≈ copy(field) |
There was a problem hiding this comment.
This sounds like a ClimaCore bug (that we cannot compare these two fields)
Purpose
Specify an exchange grid from the coupler, and make the land space independent of the atmosphere space. This also necessitates meaningful remapping, rather than the
dummmy_remap!function that has been used so far. This PR introduces non-conservative, MPI-aware (but inefficient) remapping.Note that this PR worsens conservation, since the atmosphere and land are now using different spaces, and we don't have conservative remapping. This will be improved once we have conservative remapping.
Requires MPI support enabled in ClimaLand v0.16.1
closes #1282
Content
update_field!methodsUtilities.swap_space!- replace withInterfacer.remap!