run AMIP with the integrated land model #1254
Conversation
WalkthroughThe changes update the continuous integration pipeline to disable many experimental, test, and integration steps while adding two new GPU tests. The simulation code in the ClimaEarth experiments has been enhanced to support a new land model option via a command-line flag and conditional logic in the simulation setup, including updates to function signatures and test invocation. Additionally, relevant argument parsing, component functions, and configuration files have been modified or added to support both the "bucket" and "integrated" land models, and a debug plotting command has been disabled. Changes
🪧 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[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 (14)
.buildkite/pipeline.yml (9)
35-41: Review of Commented ClimaCore Environment Setup:
The previously active steps for instantiating the ClimaCore experiments environment are now commented out. Please confirm that these steps are intentionally disabled in favor of the integrated land model run. If these steps will never be needed again, consider removing them to reduce clutter.
49-54: Review of Commented Test Environment Setup:
The commands for instantiating the test environment have been commented out. Ensure that this is intentional for the current simulation run and that any future reactivation of these tests is well documented.
67-79: Review of Commented Unit Tests Group:
The entire "Unit Tests" group—including the MPI Utilities tests—is commented out. Verify that these tests are not required for the AMIP integrated land simulation or that they are temporarily disabled pending further updates.
80-91: Review of Commented GPU Unit Tests:
The GPU unit tests group is commented out. Confirm that these tests are not essential for the current CI run, especially since the focus has shifted towards GPU integration tests tailored for AMIP.
95-104: Review of Commented GPU Slabplanet Tests:
The GPU tests for Slabplanet—and specifically the albedo-related functionality—are commented out. Please ensure that if these tests are deprecated or temporarily disabled, the documentation is updated accordingly to avoid confusion.
133-141: Review of Commented GPU AMIP Albedo Test:
The GPU AMIP test for “albedo from function” is commented out. Verify whether this test is obsolete or is simply paused for later use. If it is no longer needed, consider removing the commented block to keep the pipeline file clean.
350-358: Review of Commented Restart Run Test:
The test for restarting and monitoring an sbatch job is currently commented out. Confirm that this is deliberate for the current simulation workflow. If these steps will not be used in the near future, removing them might improve readability.
360-397: Review of Commented Hierarchy Tests:
The hierarchy tests for configurations such as Dry Held Suarez, Moist Held Suarez, Cloudless Aquaplanet, Cloudy Aquaplanet, and Cloudy Slabplanet are commented out. Please verify that these tests are either deprecated or will be re-enabled later; if the former, consider cleaning them up to simplify maintenance.
416-422: Review of Commented Hierarchy Plots Step:
The step for generating hierarchy plots is commented out. Confirm whether this step is scheduled for reactivation in future iterations or if it should be removed permanently to streamline the pipeline.experiments/ClimaEarth/Manifest-v1.11.toml (2)
315-318: Dependency Update for ClimaCore:
The record for ClimaCore has been updated with a new commit hash and its version is bumped to “0.14.29”. This update appears to bring the dependency in line with the latest changes required by the integrated land model.
345-350: Dependency Update for ClimaLand:
The ClimaLand block now shows an updated git-tree-sha1 and a version bump to “0.15.13”. In addition, the change in the extensions (e.g. NeuralSnowExt now no longer lists cuDNN) indicates a cleanup of weak dependencies. Please ensure that these modifications align with the expectations of the integrated land model and that any calling code uses the updated API.experiments/ClimaEarth/components/land/climaland_integrated.jl (3)
416-418: Clarify if:air_humidityupdates specific or relative humidity.
Here we assignsim.integrator.p.drivers.qyet the code referencesVal(:specific_humidity)later. Consider renaming:air_humidityto:specific_humidityor adding clarifying comments.-function Interfacer.update_field!(sim::ClimaLandSimulation, ::Val{:air_humidity}, field) +function Interfacer.update_field!(sim::ClimaLandSimulation, ::Val{:specific_humidity}, field)
445-461: Parameterarea_fractionis unused inupdate_sim!.
The function signature declaresarea_fraction, but the parameter is never referenced in the body. Consider removing to avoid confusion, or incorporate it if needed.-function FieldExchanger.update_sim!(sim::ClimaLandSimulation, csf, turbulent_fluxes, area_fraction) +function FieldExchanger.update_sim!(sim::ClimaLandSimulation, csf, turbulent_fluxes)
574-629: Optimize for in-place updates.
coupler_land_turbulent_fluxes!acknowledges a known allocation concern. Consider refactoring to reduce allocations by reusing buffers for repeated flux computations.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
.buildkite/pipeline.yml(5 hunks)config/ci_configs/amip_integrated_land.yml(1 hunks)experiments/ClimaCore/Manifest-v1.11.toml(13 hunks)experiments/ClimaCore/Manifest.toml(15 hunks)experiments/ClimaEarth/Manifest-v1.11.toml(13 hunks)experiments/ClimaEarth/Manifest.toml(15 hunks)experiments/ClimaEarth/cli_options.jl(1 hunks)experiments/ClimaEarth/components/atmosphere/climaatmos.jl(1 hunks)experiments/ClimaEarth/components/land/climaland_integrated.jl(8 hunks)experiments/ClimaEarth/setup_run.jl(3 hunks)experiments/ClimaEarth/test/component_model_tests/climaatmos_coarse_short.yml(1 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/postprocessing.jl(1 hunks)src/FieldExchanger.jl(2 hunks)src/FluxCalculator.jl(4 hunks)test/field_exchanger_tests.jl(1 hunks)
⏰ 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 (61)
experiments/ClimaEarth/cli_options.jl (1)
142-145: LGTM - CLI option for land model selection added correctlyThe CLI option for selecting between different land models is well-implemented with clear documentation. The default is set to "bucket" with "integrated" as an alternative option, which aligns with the PR objectives.
experiments/ClimaEarth/user_io/arg_parsing.jl (2)
117-117: LGTM - Land model configuration parameter properly extractedThe land_model parameter is correctly extracted from the configuration dictionary.
146-146: LGTM - Land model parameter properly included in return tupleThe land_model parameter is correctly included in the return tuple, making it available to consumers of this function.
experiments/ClimaEarth/components/atmosphere/climaatmos.jl (1)
233-238: Improved CO2 retrieval logic with clear documentationThe implementation now handles both CO2 storage methods (FixedCO2 and MaunaLoa) with a clean conditional expression and clear explanatory comments. This change enhances the flexibility of the code.
experiments/ClimaEarth/setup_run.jl (2)
66-66: Including the integrated land model fileThe inclusion of
climaland_integrated.jladds support for the integrated land model option, which aligns with the PR objective of setting up an AMIP simulation with the integrated land model.
340-375: Well-structured land model selection logicThe code now supports multiple land model types through a clean conditional structure. This implementation:
- Initializes
land_simtonothing- Provides clear options for "bucket" or "integrated" land models
- Properly handles parameter differences between models
- Includes error handling for invalid model types
This implementation enhances the flexibility of the simulation setup and follows good practices for extensibility.
One observation: the
ClimaLandSimulationconstructor doesn't include theenergy_checkandparameter_filesparameters that are used with theBucketSimulation. This appears intentional but should be verified as the correct behavior.experiments/ClimaEarth/test/component_model_tests/climaland_tests.jl (2)
3-14: Updated imports and includes for integrated land model supportThe imports and includes have been properly updated to support the integration of the land model with ClimaAtmos. The addition of
ClimaAtmosimport and appropriate backend handling forClimaCommsis necessary for the new test functionality.
56-107: Comprehensive test for ClimaLandSimulation flux calculationsThis new test set thoroughly validates the interaction between the atmospheric and land simulations:
- Sets up both simulation components with appropriate parameters
- Creates necessary coupler fields for data exchange
- Executes a series of field exchanges to test the interface
- Verifies that computed fluxes are non-zero
The test provides excellent coverage of the new functionality and will help ensure the integrated land model works correctly with the atmospheric model.
test/field_exchanger_tests.jl (1)
210-222: Improved naming and expanded test coverage for field exchangeThe changes enhance clarity and test coverage by:
- Renaming
atmos_namesto the more genericcomponent_namesto better reflect its purpose- Creating
component_fieldsusing these names- Adding
land_simtomodel_simsto test interaction with land componentsThese changes align well with the expanded functionality in
FieldExchanger.jlthat now supports multiple component types.src/FieldExchanger.jl (2)
69-74: Enhanced field exchange between multiple simulation componentsThe
import_atmos_fields!function has been modified to iterate over all components inmodel_sims, allowing for a more flexible and comprehensive field exchange process. This change supports the integration of multiple component models, including the new integrated land model.
76-102: Well-designed specialized method implementationsThe addition of specialized methods for
import_atmos_fields!with different simulation types:
- Provides a clear implementation for
SurfaceModelSimulationtypes- Defines a no-op implementation for
AtmosModelSimulationtypesThis approach follows good object-oriented design principles by implementing type-specific behavior, which enhances the modularity and extensibility of the code.
.buildkite/pipeline.yml (1)
123-131: Review of GPU AMIP: Integrated Land Test:
A new GPU test labeled "GPU AMIP: integrated land" has been added. The command correctly references the new configuration file (amip_integrated_land.yml) and uses a dedicated job ID. This aligns well with the PR objectives for running AMIP with the integrated land model.config/ci_configs/amip_integrated_land.yml (1)
1-25: Review of New AMIP Integrated Land Configuration File:
This new configuration file defines the simulation parameters needed for the integrated land model in AMIP runs. Key points include:
- Solver and model options such as
apply_limiter,approximate_linear_solve_iters, andco2.- Time step settings with clear values for
dt,dt_cpl,dt_rad, anddt_save_to_sol.- Vertical grid definitions (
dz_bottomanddz_top) and the diffusive flux toggle (edmfx_sgs_diffusive_flux) are appropriately set.- The
land_modelparameter is set to"integrated", which meets the PR objectives.- Additional simulation parameters like
mode_name,moist,precip_model,rad,rayleigh_sponge, andt_endare provided with intended values.- Extra options regarding turbulent processes and grid elements are present and consistent.
Overall, the configuration appears complete and correctly aligned with the updated simulation requirements.experiments/ClimaCore/Manifest-v1.11.toml (9)
280-282: ClimaCore Dependency Update:
The manifest now shows a new commit hash (36577494c1b57504130140b764de85d537c28e3f) and bumps the version to 0.14.29 for ClimaCore. Please verify that this update has been tested with the integrated land model simulation and that all dependent modules remain compatible.
488-490: DiffEqBase Update:
The git-tree-sha1 has been updated and the version is bumped to 6.167.2. Ensure that this change is consistent with your simulation requirements and that no regressions have been introduced in differential equation routines.
544-546: DifferentiationInterface Update:
The updated commit hash (70e500f6d5d50091d87859251de7b8cd060c1cce) and version change to 0.6.50 for DifferentiationInterface have been applied. Confirm that the new version interfaces correctly with the AD infrastructure used in the project.
612-614: DocStringExtensions Update:
The manifest reflects a new git-tree-sha1 and a bumped version to 0.9.4 for DocStringExtensions. Since these files are machine-generated, please double-check that the documentation tooling is still aligned with the expected API outputs.
703-705: FastPower Update:
FastPower has been updated with a new commit hash and bumped to version 1.1.2. Ensure that this change does not affect performance-critical routines that depend on FastPower, especially in numerical computations relating to the AMIP simulation.
809-811: FreeType2_jll Update:
The update to FreeType2_jll (new git-tree-sha1 and version 2.13.4+0) is reflected here. Please confirm that this version is compatible with the rendering or image-processing components that might be part of visualization or logging tasks during the simulation runs.
1545-1547: PDMats Update:
The PDMats dependency now shows an updated commit hash and its version has been bumped to 0.11.33. Verify that any numerical linear algebra operations relying on PDMats continue to perform as expected.
1720-1722: RecursiveArrayTools Update:
The changes applied (new commit hash and version 3.31.2) for RecursiveArrayTools should help with compatibility and performance in array-related operations. It is recommended to check that any custom array manipulations in the integrated simulation work seamlessly with this updated version.
2090-2093: TaylorSeries Update:
The TaylorSeries dependency update includes new settings for the commit hash and an updated version of 0.18.4. This update is critical if any automatic differentiation or series expansion routines are used. Please ensure that the numerical properties and performance improvements (if any) have been verified in the simulation context.src/FluxCalculator.jl (7)
28-29: Export list updated with new functions.The export list has been updated to include
water_albedo_from_atmosphere!andcompute_surface_fluxes!functions, making them available to importers of this module.
131-132: Parameter name improved for clarity.Renaming parameter
fieldstocsfimproves clarity by better reflecting that it contains coupler surface fields. This aligns with the function documentation on line 116.
145-148: Refactored flux calculation to improve modularity.The previous inline implementation of flux calculations has been replaced with a call to the new
compute_surface_fluxes!function. This improves code modularity and readability by encapsulating the flux calculation logic in a dedicated function.
150-151: Added explicit return statement for clarity.Adding an explicit
return nothingat the end of the function improves code clarity.
385-418: Well-documented first overload for atmospheric model simulation.The new
compute_surface_fluxes!function is thoroughly documented with clear explanations of its purpose, arguments, and behavior. The first overload correctly handles the case for an atmospheric model simulation by doing nothing, which is appropriate since an atmospheric model doesn't need to compute flux contributions to itself.
420-474: Comprehensive implementation for surface model simulations.The second overload of
compute_surface_fluxes!for surface model simulations provides a thorough implementation that:
- Retrieves necessary fields from both the atmosphere and surface models
- Computes area fractions and masks
- Determines surface thermodynamic state
- Sets up inputs based on the surface scheme
- Calculates and updates surface fluxes
- Correctly applies area weighting to flux contributions
The area weighting approach ensures that fluxes from different surface models are properly combined in proportion to their coverage area.
469-472: Area-weighted flux contributions correctly implemented.The implementation correctly applies both area fraction and area mask when adding flux contributions to the coupler fields. This ensures that:
- Surfaces contribute proportionally to their coverage area
- Surfaces with zero area fraction don't contribute at all
This approach properly handles mixed surface grid cells in the coupled model.
experiments/ClimaEarth/Manifest-v1.11.toml (4)
665-668: DocStringExtensions Version Bump:
The updated git-tree-sha1 and version “0.9.4” for DocStringExtensions have been incorporated. This update should be reviewed to ensure that changes in documentation-related functionality do not conflict with how documentation is generated or parsed in the rest of the project.
757-761: FastPower Dependency Update:
FastPower now reports a new commit hash with its version bumped to “1.1.2”. Verify that all associated extension hooks (e.g. for Enzyme, ForwardDiff, etc.) are still valid with this update.
1218-1223: JSON3 Update:
The JSON3 dependency now uses the updated git-tree-sha1 and has its version increased to “1.14.2”. This change should help maintain compatibility with any JSON parsing or serialization routines elsewhere in the framework.
1-2666: Overall Manifest Consistency Check:
This manifest file—generated automatically—includes extensive updates: dependencies are incremented, some obsolete dependencies such as ArtifactWrappers have been removed, and commit hashes updated to ensure consistency across the project. Please double-check that these version bumps and hash updates are in line with those in the corresponding ClimaCore and ClimaLand manifest files so that the integrated simulation runs smoothly with the new land model.experiments/ClimaEarth/test/component_model_tests/climaatmos_coarse_short.yml (1)
1-27: New YAML Configuration for AMIP Simulation Test:
This new configuration file clearly defines all the simulation parameters (e.g. timestep, grid spacing, diffusion options) needed for running the AMIP simulation with the integrated land model test. In particular, note that many duration values (such as “120secs”, “1hours”, and “240secs”) appear as bare strings. It would be good to verify that the simulation framework correctly parses these unit strings.experiments/ClimaEarth/Manifest.toml (9)
3-3: Update Julia version.
Thejulia_versionhas been updated to"1.10.9", which reflects the maintenance update from the previous version. Given that this file is machine‐generated, ensure that downstream build and CI processes are consistent with this version.
312-314: Update ClimaCore dependency.
Both thegit-tree-sha1(line 312) and theversion(line 314) for theClimaCoredependency have been updated. Please verify that these new identifiers and version numbers properly synchronize with the updated upstream repository (now at version"0.14.29").
343-346: Update ClimaLand dependency.
The dependency block forClimaLandshows three changes: the dependency list (line 343), thegit-tree-sha1(line 344), and theversion(line 346, now"0.15.13"). Double-check that the revised dependencies (and any removals or additions) align with the integrated land model requirements and that no unintended dependencies were dropped.
661-663: Update DocStringExtensions dependency.
Thegit-tree-sha1(line 661) and theversion(line 663) forDocStringExtensionshave been updated to reflect version"0.9.4". Please ensure that any code relying on documentation extensions remains compatible with this update.
753-755: Update FastPower dependency.
The changes on line 753 (updatedgit-tree-sha1) and line 755 (version now"1.1.2") forFastPowerindicate an update. Verify that no breaking changes have been introduced in FastPower that might affect symbolic differentiation or related numerical routines.
677-679: Update EnumX dependency.
Thegit-tree-sha1(line 677) andversion(line 679) forEnumXhave been updated to"1.0.5". Please confirm that the new build is compatible with other modules that rely on enum support in the project.
1212-1214: Update JSON3 dependency.
The modifications at line 1212 (updatedgit-tree-sha1) and line 1214 (version now"1.14.2") forJSON3reflect a dependency update. It is advisable to test data serialization and JSON parsing workflows to verify compatibility with the new version.
586-588: Update DiffEqBase dependency.
Thegit-tree-sha1(line 586) and theversion(line 588, now"6.167.2") forDiffEqBasehave been updated. Given the central role of DiffEqBase in numerical simulations, please ensure that any changes in its API or numerical behavior are fully validated in your simulation tests.
852-854: Update FreeType2_jll dependency.
The update on line 852 (newgit-tree-sha1) and line 854 (version now"2.13.4+0") forFreeType2_jllindicates an update to the FreeType library wrapper. Confirm that font rendering or related image-processing workflows that depend on FreeType continue to function as expected with this update.experiments/ClimaEarth/components/land/climaland_integrated.jl (9)
3-3: No immediate concerns with the new import.
This import brings inClimaLand.Parameters as LPwhich is used below. No issues are apparent.
120-120: Parameter usage clarity check.
atmos = CL.CoupledAtmosphere{FT}(boundary_space)looks fine; just verify thatboundary_spaceis appropriate for your domain and does not require additional checks or transformations.
423-429: Double-check usage ofLP.ρ_cloud_liqfor precipitation density.
It appears to retrieve cloud liquid density for precipitation conversion. This might be correct, but please confirm that it matches your physical assumptions for liquid precipitation density.
442-443: Integrator bridging methods look consistent.
The delegated calls toInterfacer.step!andInterfacer.reinit!appear correct. No issues noted.
489-490: Documentation update looks good.
The added bullet points for:P_liqand:P_snowclarify moisture flux field usage.
493-494: Expanded coupler fields for moisture flux.
The array now includes:P_liqand:P_snow; this aligns with the prior documentation changes.
504-505: Comment line only.
These lines simply mark an extension for land-specific flux calculations. No concerns.
406-406:✅ Verification successful
Confirm the switch to
sim.integrator.p.drivers.frac_diffis correct.
Previously,diffuse_fractionmight have been stored elsewhere. Ensure the integrator similarly usesfrac_diffin place of the old structure for consistent radiative transfer updates.
🏁 Script executed:
#!/bin/bash # Check references to frac_diff throughout the codebase to confirm consistent usage rg "frac_diff"Length of output: 314
Integrator switch to
frac_diffconfirmedThe investigation shows that the code and tests consistently reference
frac_diff(as seen insim.integrator.p.drivers.frac_diffand in the test file verifying driver names). There is no evidence of the olddiffuse_fractionbeing used elsewhere in the codebase. The update appears correct and maintains the consistency required for the radiative transfer updates.
506-572:❓ Verification inconclusive
Validate flux shapes with partial area coverage.
compute_surface_fluxes!accumulates fluxes via@. csf.<field> += ... * area_fraction * area_mask. Ifarea_fractionorarea_maskcan exceed [0, 1], consider clamping or runtime checks.
🏁 Script executed:
#!/bin/bash # Search for definitions or references of area_mask/Utilities.binary_mask to confirm it clamps values properly rg "binary_mask"Length of output: 1866
Ensure Area Fraction Validity for Flux Weighting
- The function accumulates fluxes using the product
area_fraction * area_mask, wherearea_maskis produced viaUtilities.binary_maskwhich, as confirmed by tests, returns either 0 or 1.- However, the implementation of
binary_maskdoes not clamp or validate the range of its input. This means that ifarea_fractioncan exceed the [0, 1] range, the weighted fluxes may be mis-scaled.- Please verify that the
area_fractionfield is indeed constrained within [0, 1] upstream of this calculation. If there’s any risk of it exceeding that range under partial area coverage, consider implementing clamping or adding runtime assertions.experiments/ClimaCore/Manifest.toml (9)
3-3: Updated Julia Version
The manifest now setsjulia_versionto"1.10.9", which is a bump from the previous version. Please ensure that all CI configurations and downstream components are compatible with this new Julia version.
277-279: ClimaCore Dependency Update
The ClimaCore dependency now references commit"36577494c1b57504130140b764de85d537c28e3f"with its version bumped to"0.14.29". Verify that this update remains backward compatible with simulation components, including the integrated land model, and that no unexpected side effects occur.
484-487: DiffEqBase Dependency Update
The DiffEqBase package now uses commit"e384a2cf3bb402e6dc66b1503ade22c7c1471c4d"and its version has been incremented to"6.167.2". Please ensure that any differential equation solvers or related algorithms that depend on DiffEqBase continue to work as expected with this new version.
540-542: DifferentiationInterface Update
The DifferentiationInterface dependency has been updated to use commit"70e500f6d5d50091d87859251de7b8cd060c1cce"with its version bumped to"0.6.50". Verify that any automatic differentiation routines or related functionalities are unaffected by this update.
607-609: DocStringExtensions Update
The DocStringExtensions package now reflects a new commit ("e7b7e6f178525d17c720ab9c081e4ef04429f860") and its version is updated to"0.9.4". As this package primarily handles documentation enhancements, ensure that the auto-generated documentation and any related tooling are updated accordingly.
623-625: EnumX Dependency Update
The EnumX dependency now uses commit"bddad79635af6aec424f53ed8aad5d7555dc6f00"and its version has been bumped to"1.0.5". Please verify that any code relying on EnumX behaves correctly with these updated definitions.
697-700: FastPower Package Update
FastPower has been updated with a new commit (referenced by"df32f07f373f06260cd6af5371385b5ef85dd762") and its version is now"1.1.2". Confirm that numerical computations, especially those involving exponentiation, yield accurate results with the new changes.
1529-1533: PDMats Dependency Bump
The PDMats package now points to commit"48566789a6d5f6492688279e22445002d171cf76"and its version has been updated to"0.11.33". Please ensure that matrix operations and distribution calculations that depend on PDMats remain stable under this update.
2056-2060: TaylorSeries Package Update
The TaylorSeries dependency has been updated with commit"7a919c8e612bd385daf79d071d246df12491e0dc"and its version is bumped to"0.18.4". Verify that series computations and any performance-critical routines relying on TaylorSeries continue to operate as expected.
| # TODO can't make debug plots because we have some NaNs | ||
| # !CA.is_distributed(cs.comms_ctx) && debug(cs, artifact_dir) |
There was a problem hiding this comment.
Debug plots disabled due to NaN values
While commenting out the debug plots is a reasonable temporary solution, the presence of NaNs in the data suggests there might be underlying issues with the integrated land model implementation that should be addressed.
Could you investigate the source of the NaN values? This might indicate problems with data initialization or calculations in the integrated land model.
| function FieldExchanger.import_atmos_fields!(csf, sim::ClimaLandSimulation, atmos_sim, turbulent_fluxes) | ||
| FieldExchanger.dummmy_remap!(csf.diffuse_fraction, Interfacer.get_field(atmos_sim, Val(:diffuse_fraction))) | ||
| FieldExchanger.dummmy_remap!(csf.SW_d, Interfacer.get_field(atmos_sim, Val(:SW_d))) | ||
| FieldExchanger.dummmy_remap!(csf.LW_d, Interfacer.get_field(atmos_sim, Val(:LW_d))) | ||
| FieldExchanger.dummmy_remap!(csf.cos_zenith_angle, Interfacer.get_field(atmos_sim, Val(:cos_zenith))) | ||
| FieldExchanger.dummmy_remap!(csf.P_air, Interfacer.get_field(atmos_sim, Val(:air_pressure))) | ||
| FieldExchanger.dummmy_remap!(csf.T_air, Interfacer.get_field(atmos_sim, Val(:air_temperature))) | ||
| FieldExchanger.dummmy_remap!(csf.q_air, Interfacer.get_field(atmos_sim, Val(:specific_humidity))) | ||
| FieldExchanger.dummmy_remap!(csf.P_liq, Interfacer.get_field(atmos_sim, Val(:liquid_precipitation))) | ||
| FieldExchanger.dummmy_remap!(csf.P_snow, Interfacer.get_field(atmos_sim, Val(:snow_precipitation))) | ||
| # CO2 is a scalar for now so it doesn't need remapping | ||
| csf.c_co2 .= Interfacer.get_field(atmos_sim, Val(:co2)) | ||
| end |
There was a problem hiding this comment.
Mismatch in field keys for cos_zenith.
The import_atmos_fields! function remaps Val(:cos_zenith), while other areas (e.g., line 438-439) refer to Val(:cos_zenith_angle). This likely causes a mismatch in the coupling.
-FieldExchanger.dummmy_remap!(csf.cos_zenith_angle, Interfacer.get_field(atmos_sim, Val(:cos_zenith)))
+FieldExchanger.dummmy_remap!(csf.cos_zenith_angle, Interfacer.get_field(atmos_sim, Val(:cos_zenith_angle)))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function FieldExchanger.import_atmos_fields!(csf, sim::ClimaLandSimulation, atmos_sim, turbulent_fluxes) | |
| FieldExchanger.dummmy_remap!(csf.diffuse_fraction, Interfacer.get_field(atmos_sim, Val(:diffuse_fraction))) | |
| FieldExchanger.dummmy_remap!(csf.SW_d, Interfacer.get_field(atmos_sim, Val(:SW_d))) | |
| FieldExchanger.dummmy_remap!(csf.LW_d, Interfacer.get_field(atmos_sim, Val(:LW_d))) | |
| FieldExchanger.dummmy_remap!(csf.cos_zenith_angle, Interfacer.get_field(atmos_sim, Val(:cos_zenith))) | |
| FieldExchanger.dummmy_remap!(csf.P_air, Interfacer.get_field(atmos_sim, Val(:air_pressure))) | |
| FieldExchanger.dummmy_remap!(csf.T_air, Interfacer.get_field(atmos_sim, Val(:air_temperature))) | |
| FieldExchanger.dummmy_remap!(csf.q_air, Interfacer.get_field(atmos_sim, Val(:specific_humidity))) | |
| FieldExchanger.dummmy_remap!(csf.P_liq, Interfacer.get_field(atmos_sim, Val(:liquid_precipitation))) | |
| FieldExchanger.dummmy_remap!(csf.P_snow, Interfacer.get_field(atmos_sim, Val(:snow_precipitation))) | |
| # CO2 is a scalar for now so it doesn't need remapping | |
| csf.c_co2 .= Interfacer.get_field(atmos_sim, Val(:co2)) | |
| end | |
| function FieldExchanger.import_atmos_fields!(csf, sim::ClimaLandSimulation, atmos_sim, turbulent_fluxes) | |
| FieldExchanger.dummmy_remap!(csf.diffuse_fraction, Interfacer.get_field(atmos_sim, Val(:diffuse_fraction))) | |
| FieldExchanger.dummmy_remap!(csf.SW_d, Interfacer.get_field(atmos_sim, Val(:SW_d))) | |
| FieldExchanger.dummmy_remap!(csf.LW_d, Interfacer.get_field(atmos_sim, Val(:LW_d))) | |
| FieldExchanger.dummmy_remap!(csf.cos_zenith_angle, Interfacer.get_field(atmos_sim, Val(:cos_zenith_angle))) | |
| FieldExchanger.dummmy_remap!(csf.P_air, Interfacer.get_field(atmos_sim, Val(:air_pressure))) | |
| FieldExchanger.dummmy_remap!(csf.T_air, Interfacer.get_field(atmos_sim, Val(:air_temperature))) | |
| FieldExchanger.dummmy_remap!(csf.q_air, Interfacer.get_field(atmos_sim, Val(:specific_humidity))) | |
| FieldExchanger.dummmy_remap!(csf.P_liq, Interfacer.get_field(atmos_sim, Val(:liquid_precipitation))) | |
| FieldExchanger.dummmy_remap!(csf.P_snow, Interfacer.get_field(atmos_sim, Val(:snow_precipitation))) | |
| # CO2 is a scalar for now so it doesn't need remapping | |
| csf.c_co2 .= Interfacer.get_field(atmos_sim, Val(:co2)) | |
| end |
| ::Type{FT}; | ||
| dt::TT, | ||
| tspan::Tuple{TT, TT}, | ||
| start_date::Dates.DateTime, | ||
| output_dir::String, | ||
| boundary_space, | ||
| area_fraction; | ||
| area_fraction, |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Ensure that all calls match the updated function signature for ClimaLandSimulation.
The addition of area_fraction as a keyword argument is logical, but please verify usage sites and ensure that other modules or scripts correctly supply the new argument.
🏁 Script executed:
#!/bin/bash
# Searches for ClimaLandSimulation calls to confirm the new signature is followed
rg "ClimaLandSimulation\s*\(" -A 5Length of output: 3558
Update all ClimaLandSimulation calls to use keyword syntax
Please update the calls so they match the revised signature where arguments after the type parameter must be passed as keywords. For example, in
• experiments/ClimaEarth/test/component_model_tests/climaland_tests.jl
change the current positional call
- land_sim = ClimaLandSimulation(FT, dt, tspan, start_date, output_dir, boundary_space, area_fraction)
+ land_sim = ClimaLandSimulation(FT; dt=dt, tspan=tspan, start_date=start_date, output_dir=output_dir, boundary_space=boundary_space, area_fraction=area_fraction)and verify that any similar calls (such as the helper in experiments/ClimaEarth/components/land/climaland_integrated.jl) are updated accordingly. This will ensure that the new keyword requirement—including the area_fraction argument—is correctly applied across all modules.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ::Type{FT}; | |
| dt::TT, | |
| tspan::Tuple{TT, TT}, | |
| start_date::Dates.DateTime, | |
| output_dir::String, | |
| boundary_space, | |
| area_fraction; | |
| area_fraction, | |
| - land_sim = ClimaLandSimulation(FT, dt, tspan, start_date, output_dir, boundary_space, area_fraction) | |
| + land_sim = ClimaLandSimulation(FT; dt=dt, tspan=tspan, start_date=start_date, output_dir=output_dir, boundary_space=boundary_space, area_fraction=area_fraction) |
juliasloan25
force-pushed
the
js/atmos-land-sim
branch
from
895dd39 to
30826fa
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
.buildkite/pipeline.yml (6)
35-41: Intentionally Disabled ClimaCore Environment SetupThe ClimaCore experiments environment instantiation commands are commented out. Please confirm that disabling these commands is intentional for this run. If they will not be reactivated soon, consider removing them to keep the pipeline configuration lean.
49-55: Test Environment Setup DisabledThe commands for instantiating the test environment are also commented out. Ensure this is deliberate and that there is a clear plan regarding when (or if) these steps will be re-enabled.
80-91: GPU Unit Tests DisabledThe GPU unit tests block is commented out. Confirm whether this suppression is temporary and, if so, document the reason for future clarity.
124-257: Extensive Integration Tests Block Commented OutA large block covering various integration tests—including several Slabplanet and AMIP experiments—is commented out. Given that this PR is focused on running AMIP with the integrated land model, ensure that disabling these tests is aligned with current objectives. If these tests are to be reactivated later, consider adding an explanatory comment.
310-349: Additional GPU AMIP Tests DisabledAnother block of GPU AMIP-related tests is commented out. If these tests are to be resumed in the future, consider adding inline comments that clarify the conditions under which they would be re-enabled.
416-423: Hierarchy Plots Step DisabledThe hierarchy plots generation step is currently commented out. Confirm whether these plots are no longer needed or if their reactivation is planned in subsequent changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.buildkite/pipeline.yml(5 hunks)config/ci_configs/amip_integrated_land.yml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- config/ci_configs/amip_integrated_land.yml
⏰ Context from checks skipped due to timeout of 90000ms (4)
- 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
🔇 Additional comments (7)
.buildkite/pipeline.yml (7)
67-79: Unit Tests Group DisabledThe entire "Unit Tests" group is currently commented out. Please verify that unit testing is not required in the current CI run. If this is a temporary measure, consider adding an inline note explaining the rationale.
92-102: GPU Experiments / ClimaEarth Tests DisabledThe GPU experiments/ClimaEarth unit tests steps are disabled. Please verify that this decision aligns with the current priorities, and consider adding a comment to explain if these tests will be re-enabled in upcoming iterations.
103-110: ClimaEarth Test Steps DisabledThese test steps for ClimaEarth are commented out. Ensure that disabling these tests is part of the intended CI configuration, and update documentation if needed.
111-123: MPI Restarts Tests DisabledThe MPI restarts tests, which include MPI-specific configurations and job parameters, are also commented out. Please confirm that these are intentionally disabled at this time.
262-268: GPU Slabplanet Test DisabledThe GPU Slabplanet test (default configuration) is commented out. Confirm whether this test is deprecated or deferred, and update inline documentation accordingly.
300-309: New GPU AMIP Integrated Land TestA new active step labeled "GPU AMIP: integrated land" has been introduced with its own configuration. Please verify that the configuration file (
$CONFIG_PATH/amip_integrated_land.yml), job ID, and artifact paths meet the intended simulation requirements for the integrated land model. This step appears to be the focal point of the PR.
350-358: Disabled Test Restart Run and Bash ScriptThe step for submitting and monitoring an sbatch job on Caltech HPC is commented out. Please verify if this part of the pipeline is intentionally disabled and update documentation if a future reactivation is planned.
juliasloan25
force-pushed
the
js/atmos-land-sim
branch
from
30826fa to
f78c201
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
config/ci_configs/amip_integrated_land.yml (1)
19-21: Radiative Settings and RNG SeedThe
radparameter is set toallskywithclear, and both theradiation_reset_rng_seedandrayleigh_spongeflags are enabled. Although YAML allows bare strings, consider quoting theradvalue (e.g.,"allskywithclear") for improved clarity and to avoid potential parsing issues in some YAML processors.experiments/ClimaEarth/test/component_model_tests/climaland_tests.jl (2)
73-80: Coupler fluxes initialized as zero fields.Good practice initializing all coupler fluxes as zero fields on the boundary space. This ensures proper initialization before computation.
Consider adding a comment explaining what the
temp1andtemp2fields are used for, as their purpose is not immediately clear.
101-102: Surface flux computation with appropriate parameters.The
compute_surface_fluxes!call is correctly structured. Thenothingparameters suggest optional arguments that aren't needed for this test - consider adding a brief comment explaining what these parameters would be used for in a real simulation.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
config/ci_configs/amip_integrated_land.yml(1 hunks)experiments/ClimaEarth/test/component_model_tests/climaland_tests.jl(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: JuliaFormatter
experiments/ClimaEarth/test/component_model_tests/climaland_tests.jl
[error] 62-62: Command failed with exit code 1.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- 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
🔇 Additional comments (20)
config/ci_configs/amip_integrated_land.yml (9)
1-3: Simulation Model Setup ParametersThe
FLOAT_TYPE,albedo_model, andatmos_config_filesettings are clearly defined. They indicate that the simulation will operate using 32-bit floats, a specific albedo model, and a pointed atmospheric configuration file. Please verify that these selections are consistent with the simulation’s computational and model configuration requirements.
4-5: Checkpoint and Coupler ConfigurationThe
checkpoint_dtparameter is set to"720hours"and thecoupler_tomlparameter correctly specifies a list with"toml/amip.toml". Ensure that the referenced TOML file exists at the expected relative path and that its settings align with the simulation needs.
6-9: Time Step DefinitionsThe time step parameters (
dt,dt_cpl,dt_save_state_to_disk, anddt_save_to_sol) are provided with consistent unit strings (e.g.,"240secs","12hours"). Confirm that these time step values match the numerical stability requirements and performance tuning for the AMIP simulation.
10-12: Grid Resolution and Energy CheckThe grid parameter
dz_bottomis set to100.0, and theenergy_checkflag is disabled, whileh_elemis set to8. These parameters govern the vertical grid spacing and the number of elements. Verify that these values are optimal for the resolution and physics intended in this simulation.
13-15: Land Model Type and Mode SelectionThe
land_albedo_typeis configured as"map_temporal", and themode_nameis set to"amip", withmono_surfaceset tofalse. These settings appear to be well-aligned with the integration of the land model via the CLI option. Ensure that any code using these parameters (e.g., via the new--land_modelflag) interprets them as intended.
16-18: NetCDF and Diagnostic Output SettingsThe parameters
netcdf_interpolation_num_points,netcdf_output_at_levels, andoutput_default_diagnosticsare well specified to manage simulation output and diagnostics. Check that these settings provide the necessary resolution and data for post-simulation analysis.
22-23: Start Date and Surface SetupThe
start_dateis specified as"20100101"andsurface_setupis set to"PrescribedSurface". These settings appear well-aligned with standard simulation formatting. Please double-check that the date format matches any other temporal configurations within the project.
24-26: Simulation Duration and TopographyThe simulation duration (
t_end), topography (topography), and smoothing (topo_smoothing) are defined to establish the run length and terrain configuration. Ensure that these values are consistent with the experimental design for the integrated land scenario.
27-30: Vertical Grid and Flux PartitioningThe parameters for vertical discretization (
z_elem,z_max) and flux partitioning (turb_flux_partition,viscous_sponge) are defined appropriately. These settings are critical for realistic model behavior in the vertical column of the simulation. Verify that they have been fine-tuned based on previous simulation experiences.experiments/ClimaEarth/test/component_model_tests/climaland_tests.jl (11)
3-8: Import statements are updated to support new test functionality.The imports have been extended to include ClimaAtmos, ClimaComms, and more specific imports from ClimaCoupler (FluxCalculator, Interfacer). The conditional import for ClimaComms backends using
@staticis a good practice to handle version compatibility.
10-14: Updated file path construction and includes.The path construction is now more robust using
pkgdirandjoinpath. The includes now properly reference the component models (land and atmosphere) that will be tested together in the new test set.
27-27: Improved constructor call using keyword arguments.The update to use keyword arguments in the
ClimaLandSimulationconstructor call improves readability and maintainability.
62-70: Well-structured setup of atmospheric and land simulations.The code correctly sets up both atmospheric and land simulations:
- Initializes atmospheric simulation with a specified configuration file
- Extracts boundary space from the atmospheric simulation to create a consistent land simulation
- Creates a land simulation with the same parameters
- Bundles the simulations in a named tuple for easier access
This approach ensures consistent spatial discretization between the models, which is essential for proper coupling.
🧰 Tools
🪛 GitHub Actions: JuliaFormatter
[error] 62-62: Command failed with exit code 1.
82-86: Proper initialization of coupler fields.The code correctly initializes the coupler fields:
- Gets default field names
- Adds these fields to each simulation
- Initializes fields with appropriate type and space
This follows best practices for preparing the models for field exchange.
88-89: Stepping the atmosphere model to generate realistic values for testing.Good approach to step the atmosphere simulation once to generate non-zero wind and humidity values before attempting field exchange. This ensures the test operates with realistic data.
91-96: Comprehensive field exchange between atmosphere and land.The code tests multiple aspects of the field exchange process:
- Importing surface fields
- Importing atmospheric fields
- Updating model simulations with the exchanged fields
The comment clarifies that this also tests several underlying methods, which is good documentation.
97-99: Proper update of land cache variables.The code correctly updates the auxiliary variables in the land model after field exchange, which is essential for proper flux calculations.
104-109: Verification that flux computation produces non-zero results.Good test methodology:
- Creates a zero field as a reference
- Verifies that all computed fluxes differ from this zero field
This confirms that the flux calculation successfully produced meaningful results.
111-115: Verification of numerical stability in flux computation.Excellent additional check to ensure that none of the computed fluxes contain NaN values, which would indicate numerical instability or errors in the calculation.
56-116: Well-designed test for integrated land-atmosphere flux calculation.This test set comprehensively validates the coupling between land and atmosphere models:
- It tests the construction of both models
- It tests the field exchange mechanism
- It tests the flux computation
- It verifies both non-zero results and absence of NaNs
This directly supports the PR objective of setting up an AMIP simulation with the integrated land model by ensuring that the basic coupling mechanisms work correctly.
🧰 Tools
🪛 GitHub Actions: JuliaFormatter
[error] 62-62: Command failed with exit code 1.
| @testset "ClimaLandSimulation flux calculations" begin | ||
| dt = Float64(120) | ||
| tspan = (Float64(0), 3.0dt) | ||
| start_date = Dates.DateTime(2008) | ||
| output_dir = pwd() | ||
|
|
There was a problem hiding this comment.
Fix formatter error on line 62.
There's a JuliaFormatter error on line 62. In Julia, it's common practice to have consistent spacing between code blocks.
-@testset "ClimaLandSimulation flux calculations" begin
- dt = Float64(120)
- tspan = (Float64(0), 3.0dt)
- start_date = Dates.DateTime(2008)
- output_dir = pwd()
-
+@testset "ClimaLandSimulation flux calculations" begin
+ dt = Float64(120)
+ tspan = (Float64(0), 3.0dt)
+ start_date = Dates.DateTime(2008)
+ output_dir = pwd()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @testset "ClimaLandSimulation flux calculations" begin | |
| dt = Float64(120) | |
| tspan = (Float64(0), 3.0dt) | |
| start_date = Dates.DateTime(2008) | |
| output_dir = pwd() | |
| @testset "ClimaLandSimulation flux calculations" begin | |
| dt = Float64(120) | |
| tspan = (Float64(0), 3.0dt) | |
| start_date = Dates.DateTime(2008) | |
| output_dir = pwd() |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
.buildkite/pipeline.yml (7)
35-41: Clarify Removal of ClimaCore Experiments Environment Setup
The ClimaCore experiments environment setup commands are now commented out. Please add a brief comment explaining whether these commands are deprecated, superseded by another mechanism, or temporarily disabled for this AMIP simulation run. This will help future maintainers understand the rationale behind their removal.
49-55: Provide Rationale for Commenting Out Test Environment Setup
The commands for instantiating the test environment are commented out. It would be helpful to document the reason (e.g., these commands are no longer necessary or they interfere with the new CI workflow) so that other team members can quickly grasp whether they should be re-enabled or removed entirely.
67-79: Commented Out Unit Tests Group
The entire "Unit Tests" group is commented out. If this is an intentional change resulting from the focus on AMIP integrations, please add a note to clarify the long-term plan for this group (for example, if they will be reintroduced later or have been replaced by alternative tests).
80-91: Intentional Disabling of GPU Unit Tests
The GPU unit tests are currently disabled. If these have been intentionally sidelined—perhaps due to resource constraints or a shift in testing strategy—please include a comment explaining the intent, so that future contributors understand the context.
92-102: Verify Necessity of Disabled ClimaEarth Unit Tests
The group for ClimaEarth unit tests and the global bucket tests is commented out. Confirm that these tests are either obsolete or will be reconfigured later. Adding a clarifying comment on the status and future plan for these tests would improve maintainability.
374-382: Clarify Disabled SBATCH Job Test
The test group for submitting and monitoring an SBATCH job on Caltech HPC is commented out. If this test is no longer applicable or is being replaced by another mechanism, please add a comment to clarify the decision. Otherwise, consider whether it should remain in the pipeline for long-term testing purposes.
440-447: Review Comment on Disabled Hierarchy Plots Step
The hierarchy plots generation step is commented out. If plotting is not needed for the current AMIP simulation runs or is being handled via an alternative process, please document this so that others will understand the intent behind this removal.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.buildkite/pipeline.yml(4 hunks)config/ci_configs/amip_decay_with_height_bucket.yml(1 hunks)config/ci_configs/amip_decay_with_height_integrated_land.yml(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- config/ci_configs/amip_decay_with_height_integrated_land.yml
- config/ci_configs/amip_decay_with_height_bucket.yml
⏰ Context from checks skipped due to timeout of 90000ms (6)
- 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
🔇 Additional comments (4)
.buildkite/pipeline.yml (4)
193-198: Active Integration Test: AMIP with Integrated Land Model
The integration test step for "AMIP: DecayWithHeight + integrated land" is added correctly with a clear command and configuration file reference. Please verify that the referenced configuration file (config/ci_configs/amip_decay_with_height_integrated_land.yml) exists and is correctly maintained. Also, confirm that the 20GB memory allocation meets the experiment’s computational requirements.
200-206: Active Integration Test: AMIP with Bucket Land Model
This test step mirrors the integrated land test and uses the bucket configuration. Ensure that theamip_decay_with_height_bucket.ymlfile is up to date and that the resource parameters (e.g., 20GB memory) are suitable for the bucket land model simulation.
314-323: GPU Integration Test: AMIP with Integrated Land Model
The GPU test for the integrated land model is set up with the appropriate CUDA environment and resource allocation. Please double-check that the GPU-specific configuration is fully compatible with the integrated land simulation and that the extra environment variable (CLIMACOMMS_DEVICE: "CUDA") is properly handled by downstream scripts.
324-333: GPU Integration Test: AMIP with Bucket Land Model
As with the CPU test, the GPU test for the bucket land model is configured similarly. Verify that the configuration file for the bucket model is correct and that the allocation of 20GB memory and one GPU is sufficient for the simulation’s workload.
juliasloan25
force-pushed
the
js/atmos-land-sim
branch
from
860bbac to
0be844b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
experiments/ClimaEarth/setup_run.jl (1)
339-374: Implementation looks good but consider parameter differences.The conditional initialization for different land models is well-structured with proper error handling for invalid model types. Both the bucket and integrated land model implementations receive appropriate parameters.
Note that the integrated land model doesn't receive the
energy_checkandparameter_filesparameters that the bucket model does. Ensure this difference is intentional and doesn't impact energy conservation tracking in the integrated model.Could you confirm that the omission of these parameters is intentional for the integrated land model? If these parameters should be included for consistency, consider adding them to the
ClimaLandSimulationinitialization..buildkite/pipeline.yml (1)
35-430: Verify the need for commented-out pipeline steps.A significant portion of the CI pipeline has been commented out, including unit tests, ClimaEarth tests, and integration tests. While this might reduce CI load during development, consider whether this could potentially miss regressions in other areas.
If this is a temporary change for development purposes, you might want to add a TODO comment or issue to restore these tests before final merging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
.buildkite/pipeline.yml(4 hunks)config/ci_configs/amip_decay_with_height_bucket.yml(1 hunks)config/ci_configs/amip_decay_with_height_integrated_land.yml(1 hunks)experiments/ClimaEarth/cli_options.jl(1 hunks)experiments/ClimaEarth/components/land/climaland_integrated.jl(2 hunks)experiments/ClimaEarth/setup_run.jl(3 hunks)experiments/ClimaEarth/test/component_model_tests/climaland_tests.jl(1 hunks)experiments/ClimaEarth/user_io/arg_parsing.jl(2 hunks)experiments/ClimaEarth/user_io/postprocessing.jl(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- experiments/ClimaEarth/user_io/postprocessing.jl
- experiments/ClimaEarth/test/component_model_tests/climaland_tests.jl
- config/ci_configs/amip_decay_with_height_integrated_land.yml
- experiments/ClimaEarth/user_io/arg_parsing.jl
- experiments/ClimaEarth/cli_options.jl
- experiments/ClimaEarth/components/land/climaland_integrated.jl
- config/ci_configs/amip_decay_with_height_bucket.yml
⏰ 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: test (1.11)
- GitHub Check: ci 1.10 - macOS-latest
- GitHub Check: ci 1.10 - ubuntu-latest
🔇 Additional comments (4)
experiments/ClimaEarth/setup_run.jl (2)
66-66: Correctly added include statement for integrated land model.The addition of the include statement for
climaland_integrated.jlproperly enables the use of the new integrated land model in the simulation setup.
197-197: Land model parameter correctly added to function parameters.The
land_modelparameter has been correctly extracted from the configuration dictionary, allowing for selection between different land model implementations..buildkite/pipeline.yml (2)
193-206: Good addition of both integrated land and bucket model tests.The addition of two CPU-based AMIP tests with the decay-with-height configuration, one for the integrated land model and one for the bucket model, will ensure both implementations are properly tested.
314-333: GPU tests correctly mirror CPU tests for both land models.The GPU tests for both the integrated land model and bucket model configurations ensure the implementation works correctly in GPU environments, which is essential for performance testing and validation.
Purpose
Set up an AMIP simulation using the integrated land model and try running it! Let's see what happens.
This is currently branched off of #1220 and should be merged after that.
To-do
land_modelSummary by CodeRabbit
New Features
Tests