Model specification in simulations struct #1140
Conversation
WalkthroughThis update introduces new functions for constructing land surface model components, including EnergyHydrology, SnowModel, SoilCO2Model, CanopyModel, and LandModel, each parameterized by floating-point type and driven by domain, parameters, and forcing data. The spatial parameter loading is refactored and modularized, splitting monolithic functions into focused routines for canopy, soil, and rooting depth parameters, with explicit masking and fallback logic. The ModelSetup module now includes the new model_setup.jl file, integrating these capabilities into the broader simulation framework. Additionally, a forcing function return type was changed from a positional tuple to a named tuple. Possibly related PRs
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: 10
♻️ Duplicate comments (1)
src/simulations/Simulations.jl (1)
118-126: Alias issue resurfaces
CTS.ClimaODEFunctionwill still fail until the alias from line 1 is added.
🧹 Nitpick comments (4)
src/integrated/land.jl (1)
173-181: Good to add explicit CO₂ BCs – tiny nit on styleNice to see the top & bottom BCs spelled out. To keep keyword ordering consistent with the rest of the codebase (and with the soil model call above) place
driversfirst and break long lines for readability.- soilco2 = soilco2_type(; soilco2_args..., boundary_conditions = soilco2_boundary_conditions, - sources = soilco2_sources,drivers = soilco2_drivers) + soilco2 = soilco2_type(; + drivers = soilco2_drivers, + boundary_conditions = soilco2_boundary_conditions, + sources = soilco2_sources, + soilco2_args..., + )src/simulations/Simulations.jl (1)
160-163: Variable name typo –_required_cbShould be
_required_callbacks.- callbacks = SciMLBase.CallbackSet(user_callbacks..., diag_cb..., _required_cb...) + callbacks = SciMLBase.CallbackSet(user_callbacks..., diag_cb..., _required_callbacks...)src/simulations/model_setup.jl (2)
7-11: Remove redundantimport ClimaLand.
using ClimaLandalready brings all names into scope; the extraimport ClimaLandis unnecessary noise.-using ClimaLand.Canopy -import ClimaLand +using ClimaLand.Canopy
12-14: Add a brief docstring forland_model_setup.A short docstring will make the intent, arguments, and return type searchable from the REPL and in docs.
+""" + land_model_setup(FT; kwargs...) + +Create and return a fully-configured `LandModel{FT}` with soil, canopy, +snow and forcing components for global simulations. +""" function land_model_setup(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/ClimaLand.jl(1 hunks)src/integrated/land.jl(1 hunks)src/shared_utilities/models.jl(2 hunks)src/shared_utilities/utils.jl(2 hunks)src/simulations/Simulations.jl(1 hunks)src/simulations/domains.jl(2 hunks)src/simulations/global_simulations_notes.jl(1 hunks)src/simulations/initial_conditions.jl(1 hunks)src/simulations/model_setup.jl(1 hunks)
🔇 Additional comments (10)
src/shared_utilities/models.jl (2)
26-27: New domain accessor function added to exportsThe
get_domainfunction has been added to the export list, making it accessible to external modules.
401-401: Simple domain accessor method implementedThis method provides a clear way to access a model's domain property, following good API design principles.
src/simulations/domains.jl (2)
47-47: Added optional communication context parameterThe
comms_ctxparameter allows specifying the communication context for domain creation, with a sensible default to the global context.
62-63: Passing communication context to SphericalShell constructorThe
dz_tupleandcomms_ctxparameters are now correctly passed to theSphericalShellconstructor.src/shared_utilities/utils.jl (2)
11-11: Added required imports for time reportingNew imports from
ClimaUtilities.OnlineLoggingsupport the wall time reporting functionality.
722-730: Added wall time reporting callbackNew
ReportCallbackfunction creates a callback that reports integration performance at specified intervals.The implementation is clean and follows the project's callback pattern. This will be useful for monitoring long-running simulations.
src/ClimaLand.jl (2)
58-74: New domain retrieval logic for land modelsThis function intelligently retrieves a domain from a multi-component land model, with priority given to subsurface domains when present.
The logic handling domain prioritization is elegant, though the long single-line implementation on line 73 could be split for better readability.
76-76: Added helper for subsurface domain detectionSimple utility function to identify if a domain is a subsurface type.
src/simulations/Simulations.jl (2)
74-81: Numerous undefined helpers & variables
construct_soil_args,construct_soil_runoff_model,param_set, etc. are not in scope, causing immediate failure. Verify imports or provide implementations before merging.
164-165: Constructor call still mismatched after struct fixEnsure argument order matches the revised struct.
-return LandSimulation(context,params, land, domain, timestepper, user_callbacks, diagnostics, outdir, _required_callbacks, problem) +return LandSimulation(context, params, land, domain, timestepper, + user_callbacks, diagnostics, outdir, + _required_callbacks, problem)
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
src/simulations/Simulations.jl (3)
75-75:⚠️ Potential issueEmpty LAI assignment breaks parsing
The LAI parameter is missing a value after the assignment operator.
- LAI = ), + LAI = LAI),
81-85:⚠️ Potential issueUndefined variable
tfin time conversionThe code uses
tfbut it's not defined as a parameter in the function.- tf = ITime(tf, epoch = start_date) + # Calculate tf based on some duration or use a parameter + tf = t0 + ITime(Month(12), epoch = start_date) # Example: 1 year simulation
47-56:⚠️ Potential issueMissing comma in tuple construction
The user_callbacks tuple is improperly constructed - elements need to be separated by commas.
- ), - ClimaLand.ReportCallback(1000) + ), + ClimaLand.ReportCallback(1000),
🧹 Nitpick comments (2)
src/simulations/global_simulations_notes.txt (1)
1-30: Design notes need formalization in actual codeThis sketch outlines the desired simulation structure but contains incomplete code fragments with trailing syntax errors and placeholders. It serves as a useful planning document, but ensure all functionality is properly implemented in the actual code.
While it's helpful to have these conceptual notes, consider turning this into proper documentation once implementation is complete.
src/simulations/Simulations.jl (1)
8-8: Missing aliases for imported modulesSeveral modules are imported but could benefit from aliases for clarity and conciseness.
+const CTS = ClimaTimesteppers import ClimaDiagnostics +const CD = ClimaDiagnostics
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Artifacts.jl(1 hunks)src/integrated/land.jl(1 hunks)src/simulations/Simulations.jl(1 hunks)src/simulations/global_simulations_notes.txt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/integrated/land.jl
🔇 Additional comments (2)
src/Artifacts.jl (2)
60-68: Docstring update improves clarityThe docstring now clearly documents the function behavior regarding resolution options and fallback mechanism.
73-92: Good implementation of fallback behavior for ERA5 dataThe function now returns specific file paths instead of directories and includes a robust fallback mechanism when high-resolution data isn't available locally.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
src/simulations/Simulations.jl (4)
93-98: Undefined construction functionsSeveral construction functions are called but not defined anywhere visible in this module.
99-104: Missing commas in tuple constructionTuple elements need commas between them.
land_input = ( - atmos = forcing[1] - radiation = forcing[2] + atmos = forcing[1], + radiation = forcing[2], runoff = runoff_model, soil_organic_carbon = soilco2_model.Csom, )
173-173: Empty tuple syntax errorEmpty tuple syntax is incorrect.
- diag_cb = (,) + diag_cb = ()
178-178: Variable name mismatch in callback collectionThe code refers to
required_cbbut the variable is defined asrequired_callbacks.- callbacks = SciMLBase.CallbackSet(user_callbacks..., diag_cb..., required_cb...) + callbacks = SciMLBase.CallbackSet(user_callbacks..., diag_cb..., required_callbacks...)
🧹 Nitpick comments (1)
src/simulations/Simulations.jl (1)
56-57: Code comments need cleanupRemove or fix the "needs to be" comments once implementation is confirmed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Artifacts.jl(1 hunks)src/simulations/Simulations.jl(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/Artifacts.jl
🔇 Additional comments (4)
src/simulations/Simulations.jl (4)
87-90: LGTM: Time conversion implementationThe time conversion and promotion logic looks good.
119-124: Solid initialization approachThe approach to initializing from a file looks clean and well-structured.
147-151: Driver callback setup looks goodThe approach for handling required callbacks is clean.
180-180: Passing the right components to LandSimulationGood job collecting all simulation components for return.
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (3)
src/simulations/model_setup.jl (2)
56-80: All helper calls use the wrong field nameSame issue as above but scattered – every occurrence of
*_model.spatially_varying_parametersmust be replaced with the key actually present in the tuple (parameters).
Failing to do so stops compilation long before run-time tests.
49-51: 🛠️ Refactor suggestionKeep the parameter struct type-stable
5is anInt; wrap it inFTto avoid implicit promotion at run time.- TimeVaryingInput((t) -> 5), + TimeVaryingInput(t -> FT(5)),src/simulations/Simulations.jl (1)
19-30:⚠️ Potential issue
Pused twice in the parametric struct – compilation blockerThe struct header defines
Pfor parameters and again for theODEProblem. Julia refuses duplicate type parameters.-struct LandSimulation{C, P, M, D, T, UC, DI, RC, CA, P} +struct LandSimulation{C, P, M, D, T, UC, DI, RC, CA, PR}and replace the last field type with
PR.
🧹 Nitpick comments (1)
src/simulations/model_setup.jl (1)
103-104: Name shadowing: callingLandModel{FT}from insideLandModelInside the function you invoke
LandModel{FT}(... ), relying on Julia to resolve to the type not the enclosing method. That works but is brittle and confusing.
Consider importing the type under another alias, e.g.CLM.LandModelor rename this helper tomake_land_model.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/simulations/Simulations.jl(1 hunks)src/simulations/domains.jl(2 hunks)src/simulations/initial_conditions.jl(1 hunks)src/simulations/model_setup.jl(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/simulations/domains.jl
- src/simulations/initial_conditions.jl
🔇 Additional comments (1)
src/simulations/Simulations.jl (1)
51-53:comms_ctxkeyword likely incorrect
ClimaLand.global_domainwas recently updated to takecontext;comms_ctxwill throwMethodErrorin currentmain.-domain = ClimaLand.global_domain(FT; comms_ctx = context), +domain = ClimaLand.global_domain(FT; context = context),
src/simulations/Simulations.jl
Outdated
| # convert times to Itime | ||
| t0 = ITime(t0, epoch = start_date) | ||
| tf = ITime(tf, epoch = start_date) | ||
| Δt = ITime(Δt, epoch = start_date) | ||
| t0, tf, Δt = promote(t0, tf, Δt) | ||
|
|
There was a problem hiding this comment.
promote(range...) is nonsensical
promote expects numbers, not a range; additionally, the splat into [] builds a vector of individual promoted elements.
Simpler & faster:
updateat = collect(t0:ITime(3600*3):tf)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
src/simulations/Simulations.jl (4)
81-86: Simplify range collectionThe complex array creation with promotion isn't needed.
-updateat = [promote(t0:(ITime(3600 * 3)):tf...)...] +updateat = collect(t0:ITime(3600*3):tf)
19-41:⚠️ Potential issueFix duplicate type parameter in LandSimulation struct
The struct uses
Ptwice - once forparamsand once for the problem type. This causes type ambiguity.- P <: SciMLBase.ODEProblem, + PR <: SciMLBase.ODEProblem,And update the field type:
- problem::P + problem::PR
66-74: 🛠️ Refactor suggestionCallbacks capture raw times before promotion to ITime
The
NaNCheckCallbackshould use the promotedITimevalues or be created after promotion.
52-56:⚠️ Potential issueMissing context parameter in LandModel construction
The model constructor should pass the context parameter.
-model = LandModel(domain, start_date, params, FT), # add assert +model = LandModel(domain, context, start_date, params, FT), # add assert
🧹 Nitpick comments (1)
src/simulations/Simulations.jl (1)
182-184: Empty plot function with incorrect indentationThe
plotfunction is empty and has indentation issues.Either implement the function or remove it. If it's a placeholder, add a TODO comment.
- function plot(landsimulation) - - end +function plot(landsimulation) + # TODO: Implement visualization of land simulation results +end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/simulations/Simulations.jl(1 hunks)src/simulations/model_setup.jl(1 hunks)src/simulations/spatial_parameters.jl(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/simulations/model_setup.jl
🔇 Additional comments (2)
src/simulations/spatial_parameters.jl (2)
191-193: Extraction of helper function is a good refactoringMoving
masked_to_valueto a top-level function makes it reusable and improves code organization.
422-445: Good separation of concernsThe new function properly isolates
f_maxloading from general soil parameters, making the code more modular and easier to maintain.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
experiments/long_runs/land_region.jl (1)
309-317: Same export concern as above
make_set_initial_state_from_fileis used here without qualification; see the
earlier comment insrc/simulations/initial_conditions.jl. Make sure the
symbol is actually in scope.experiments/long_runs/snowy_land.jl (1)
350-359: Repeat of export / visibility issueThe same unqualified call appears here; please address once globally.
🧹 Nitpick comments (4)
experiments/long_runs/soil.jl (1)
155-156: Qualify helper to avoid accidental name capture
make_set_initial_state_from_fileis referenced without a module prefix.
Even thoughusing ClimaLandis in scope, qualifying with the namespace guards
against a future local definition shadowing the helper and makes the origin
obvious:-set_initial_state! = make_set_initial_state_from_file(ic_path, soil) +set_initial_state! = ClimaLand.make_set_initial_state_from_file(ic_path, soil)test/integrated/full_land.jl (1)
234-236: Match style used elsewhere & improve claritySame remark as in
soil.jl: qualify the helper for readability and to prevent
accidental masking.-set_initial_state! = make_set_initial_state_from_file(ic_path, land) +set_initial_state! = ClimaLand.make_set_initial_state_from_file(ic_path, land)experiments/integrated/global/global_soil_canopy.jl (1)
183-191: Minor stylistic tweak – qualify helperFor consistency across files, prefix the helper with
ClimaLand.:-set_initial_state! = make_set_initial_state_from_file(ic_path, land) +set_initial_state! = ClimaLand.make_set_initial_state_from_file(ic_path, land)Everything else in the new init flow looks solid.
experiments/calibration/forward_model_land.jl (1)
355-361: Consistency: qualify helper callSame tiny tweak as above – qualify the factory call for clarity:
-set_initial_state! = make_set_initial_state_from_file(ic_path, land) +set_initial_state! = ClimaLand.make_set_initial_state_from_file(ic_path, land)No functional issues spotted.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
experiments/calibration/forward_model_land.jl(1 hunks)experiments/integrated/global/global_soil_canopy.jl(1 hunks)experiments/long_runs/land_region.jl(1 hunks)experiments/long_runs/snowy_land.jl(1 hunks)experiments/long_runs/soil.jl(1 hunks)src/shared_utilities/utils.jl(2 hunks)src/simulations/initial_conditions.jl(2 hunks)src/simulations/spatial_parameters.jl(2 hunks)test/integrated/full_land.jl(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/shared_utilities/utils.jl
- src/simulations/spatial_parameters.jl
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: lib-climalandsimulations (1.11)
- GitHub Check: lib-climalandsimulations (1.10)
- GitHub Check: downstream ClimaCoupler.jl (1.10)
- GitHub Check: downstream ClimaCoupler.jl (1.11)
- GitHub Check: ci 1.11 - ubuntu-latest
- GitHub Check: ci 1.11 - windows-latest
- GitHub Check: ci 1.10 - ubuntu-latest
- GitHub Check: ci 1.10 - windows-latest
- GitHub Check: docbuild
- GitHub Check: test (1.10)
- GitHub Check: test (1.11)
🔇 Additional comments (1)
src/simulations/initial_conditions.jl (1)
211-214:❌ Incorrect review comment
Is
make_set_initial_state_from_filere-exported?The helper scripts call the function unqualified (
make_set_initial_state_from_file(…)),
assuming it lives in the top-levelClimaLandnamespace.
This file lives underClimaLand.Simulations– unless that sub-module
explicitly re-exports the symbol, users will hit anUndefVarError.Please confirm the export path or update the call-sites to
ClimaLand.Simulations.make_set_initial_state_from_file.
🏁 Script executed:
#!/bin/bash # List modules that export the symbol rg -n "export .*make_set_initial_state_from_file" -g "*.jl"Length of output: 141
🏁 Script executed:
#!/bin/bash # Locate the top-level ClimaLand module file and check submodule inclusion/export rg -n "module ClimaLand" -g "*.jl" rg -n "include.*Simulations" -g "*.jl" rg -n "export.*Simulations" -g "*.jl"Length of output: 236
🏁 Script executed:
#!/bin/bash # Show top of src/ClimaLand.jl to verify inclusion/export of the Simulations submodule sed -n '1,200p' src/ClimaLand.jlLength of output: 7109
🏁 Script executed:
#!/bin/bash # Find unqualified calls to make_set_initial_state_from_file rg -n "make_set_initial_state_from_file" -g "*" # List simulations directory structure fd -t d src/simulationsLength of output: 1839
🏁 Script executed:
#!/bin/bash # Check the module declaration in initial_conditions.jl rg -n "module ClimaLand.Simulations" -g "*.jl" sed -n '1,20p' src/simulations/initial_conditions.jl # Search for any mention of Simulations in the top‐level module rg -n "Simulations" -g "src/ClimaLand.jl"Length of output: 1121
🏁 Script executed:
#!/bin/bash # Check if simulations files are included in the top‐level module rg -n 'include.*simulations' -g src/ClimaLand.jl # Show lines around the "Simulations" section in ClimaLand.jl sed -n '360,440p' src/ClimaLand.jl # Inspect test file for how make_set_initial_state_from_file is imported sed -n '1,100p' test/integrated/full_land.jlLength of output: 4639
No qualification needed for make_set_initial_state_from_file
The file src/simulations/initial_conditions.jl is included inside the top-level ClimaLand module (see lines 411–413 of src/ClimaLand.jl), and it contains
export make_set_initial_state_from_fileso after
using ClimaLandthe function is available unqualified. You can ignore the suggestion to fully qualify it.
Likely an incorrect or invalid review comment.
kmdeck
force-pushed
the
kd/global_simulations
branch
from
548feba to
a4aeefc
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (11)
src/simulations/model_setup.jl (5)
16-17: Same undefinedcontextissue in LAI setup.Similar to the forcing setup, context is referenced but not defined.
51-52:⚠️ Potential issue
Δtis undefined in snow_model parameters.The snow parameters constructor requires
Δtbut it's not in scope.Either add
Δtas a parameter toLandModelor provide a default value:parameters = SnowParameters{FT}(Δt; earth_param_set = params),
46-47: 🛠️ Refactor suggestionReturn
FT-typed constants for type stability.Using raw integer
5will force runtime type promotion when combined withFTvalues.Csom = ClimaLand.PrescribedSoilOrganicCarbon{FT}( - TimeVaryingInput((t) -> 5), + TimeVaryingInput((t) -> FT(5)), ),
90-91:⚠️ Potential issueField name mismatch will cause runtime error.
The code is accessing
soil_model.spatially_varying_parametersbut the struct field is namedparameters.soil_params = Soil.EnergyHydrologyParameters( FT; - soil_model.spatially_varying_parameters..., + soil_model.parameters..., )
6-7:⚠️ Potential issue
contextis undefined, leading to potential runtime error.The code attempts to use
contextinside the forcing setup, but it's not defined as an argument to theLandModelfunction.function LandModel( domain, + context, start_date, params, FT; forcing = ClimaLand.prescribed_forcing_era5( ClimaLand.Artifacts.era5_land_forcing_data2008_folder_path(; context),# get context from domainsrc/simulations/Simulations.jl (6)
178-180:⚠️ Potential issueUndefined variables in solve! function.
The function attempts to use variables not in scope.
function solve!(simulation) - SciMLBase.solve!(problem, ode_algo, callbacks) + SciMLBase.solve(simulation.problem, simulation.timestepper, callback=simulation.callbacks) end
115-116: 🛠️ Refactor suggestionIncorrect use of
promotewith range.
promoteexpects individual values, not a range expression.- updateat = [promote(t0:(ITime(3600 * 3)):tf...)...] + updateat = collect(t0:ITime(3600*3):tf)
19-30:⚠️ Potential issueType parameter
Pis used twice, causing ambiguity.The struct uses
Pboth for the params field and for the ODEProblem.struct LandSimulation{ C, P, M <: ClimaLand.AbstractLandModel, D <: ClimaLand.Domains.AbstractDomain, T <: ClimaTimeSteppers.IMEXAlgorithm, UC, DI, RC, CA <: SciMLBase.CallbackSet, - P <: SciMLBase.ODEProblem, + PR <: SciMLBase.ODEProblem, }
162-168:⚠️ Potential issueConstructor arguments don't match struct fields.
Multiple issues: wrong variable name and extra arguments not in the struct.
return LandSimulation( context, params, model, domain, timestepper, user_callbacks, diagnostics, - _required_callbacks, + required_callbacks, callbacks, problem, - t - Y, - __p #_integrator )
52-53:⚠️ Potential issueLandModel call missing the context argument.
Based on the issues in
model_setup.jl, LandModel requires a context parameter.- model = LandModel(domain, start_date, params, FT), # add assert + model = LandModel(domain, context, start_date, params, FT), # add assert
172-176:⚠️ Potential issueUndefined variables in step! function.
The function refers to variables not in scope.
function step!(landsimulation) - integrator = SciMLBase.integrator(Y, p, t, callbacks) + integrator = SciMLBase.init(landsimulation.problem, landsimulation.timestepper, callback=landsimulation.callbacks) SciMLBase.step!(integrator) - simulation.t[] = integrator.t + # Update state if needed end
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
src/Artifacts.jl(1 hunks)src/shared_utilities/utils.jl(2 hunks)src/simulations/Simulations.jl(1 hunks)src/simulations/domains.jl(2 hunks)src/simulations/global_simulations_notes.txt(1 hunks)src/simulations/model_setup.jl(1 hunks)src/simulations/spatial_parameters.jl(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/simulations/domains.jl
- src/shared_utilities/utils.jl
- src/simulations/global_simulations_notes.txt
- src/Artifacts.jl
- src/simulations/spatial_parameters.jl
🧰 Additional context used
🪛 GitHub Actions: JuliaFormatter
src/simulations/Simulations.jl
[error] 184-184: JuliaFormatter parsing error at line 184, offset 11. Formatting failed due to syntax or parsing issue in the source code.
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: ci 1.11 - windows-latest
- GitHub Check: ci 1.10 - macOS-latest
- GitHub Check: ci 1.11 - macOS-latest
- GitHub Check: ci 1.11 - ubuntu-latest
- GitHub Check: ci 1.10 - ubuntu-latest
- GitHub Check: ci 1.10 - windows-latest
- GitHub Check: docbuild
- GitHub Check: test (1.11)
- GitHub Check: test (1.10)
- GitHub Check: lib-climalandsimulations (1.10)
- GitHub Check: lib-climalandsimulations (1.11)
- GitHub Check: downstream ClimaCoupler.jl (1.10)
- GitHub Check: downstream ClimaCoupler.jl (1.11)
kmdeck
changed the title
kmdeck
force-pushed
the
kd/global_simulations
branch
from
a4aeefc to
eda4470
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/simulations/ModelSetup.jl(1 hunks)src/simulations/model_setup.jl(1 hunks)src/simulations/spatial_parameters.jl(10 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/simulations/ModelSetup.jl
🔇 Additional comments (1)
src/simulations/spatial_parameters.jl (1)
616-626:⚠️ Potential issueExtra
)breakstopmodel_fmaxdefinition
The function header ends with)), causing a parse error.- )) + )Short but fatal.
Likely an incorrect or invalid review comment.
src/simulations/model_setup.jl
Outdated
| function LandModel(FT, start_date, Δt, domain, earth_param_set; | ||
| prognostic_land_components = (:canopy, :snow, :soil, :soilco2), | ||
| forcing = ClimaLand.prescribed_forcing_era5( | ||
| ClimaLand.Artifacts.era5_land_forcing_data2008_path(; ClimaComms.context(domain)), | ||
| domain.space.surface, | ||
| start_date, | ||
| earth_param_set, | ||
| FT; | ||
| time_interpolation_method = LinearInterpolation(PeriodicCalendar()), | ||
| ), | ||
| LAI = ClimaLand.prescribed_lai_modis( | ||
| ClimaLand.Artifacts.modis_lai_single_year_path(; | ||
| context = ClimaComms.context(domain), | ||
| year = Dates.year(start_date), | ||
| ), | ||
| domain.space.surface, | ||
| start_date; | ||
| time_interpolation_method = LinearInterpolation(PeriodicCalendar()), | ||
| ), | ||
| soil = EnergyHydrology(FT, domain, earth_param_set, forcing, prognostic_land_components), | ||
| canopy = CanopyModel(FT, domain, earth_param_set, forcing, prognostic_land_components) | ||
| snow = SnowModel(FT, domain, earth_param_set, forcing, prognostic_land_components, Δt), | ||
| soilco2 = SoilCO2Model(FT, domain, earth_param_set, forcing, prognostic_land_components, soil_organic_carbon, soil_params), | ||
| ) |
There was a problem hiding this comment.
LandModel keyword list is syntactically broken & uses undeclared vars
• Missing comma after the canopy = … line.
• soil_organic_carbon and soil_params are referenced but never defined.
• SoilCO2Model therefore cannot build.
- canopy = CanopyModel(FT, domain, earth_param_set, forcing, prognostic_land_components)
- snow = SnowModel(FT, domain, earth_param_set, forcing, prognostic_land_components, Δt),
- soilco2 = SoilCO2Model(FT, domain, earth_param_set, forcing, prognostic_land_components, soil_organic_carbon, soil_params),
+ canopy = CanopyModel(FT, domain, earth_param_set, scalar_params, forcing, prognostic_land_components),
+ snow = SnowModel(FT, domain, earth_param_set, forcing, prognostic_land_components, Δt),
+ soilco2 = SoilCO2Model(
+ FT, domain, earth_param_set, forcing, prognostic_land_components;
+ soil_organic_carbon = default_SOC(domain), # supply or add arg
+ soil_params = soil.parameters,
+ ),Ensure helper vars exist or accept them as arguments.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/simulations/model_setup.jl around lines 66 to 89, the LandModel
function's keyword argument list is missing a comma after the canopy assignment,
causing a syntax error. Additionally, soil_organic_carbon and soil_params are
used in the SoilCO2Model call but are not defined or passed in. Fix this by
adding the missing comma after the canopy line and either define
soil_organic_carbon and soil_params within the function or add them as
parameters to LandModel so they can be passed in properly.
src/simulations/model_setup.jl
Outdated
| function CanopyModel(FT, domain, earth_param_set, scalar_params, forcing, prognostic_land_components; | ||
| autotrophic_respiration = Canopy.AutotrophicRespirationModel{FT}(Canopy.AutotrophicRespirationParameters(FT)), | ||
| radiative_transfer = Canopy.TwoStreamModel{FT}(Canopy.TwoStreamParameters(FT, domain, scalar_params)), | ||
| photosynthesis = Canopy.FarquharModel{FT}(Canopy.FarquharParameters(FT, domain, scalar_params)), | ||
| conductance = Canopy.MedlynConductanceModel{FT}(Canopy.MedlynConductanceParameters(FT, domain, scalar_params)), | ||
| hydraulics = Canopy.PlantHydraulicsModel{FT}(Canopy.PlantHydraulicsParameters(scalar_params)), | ||
| energy = Canopy.BigLeafEnergyModel{FT}(Canopy.BigLeafEnergyParameters{FT}(scalar_params)), | ||
| parameters = Canopy.SharedCanopyParameters{FT, typeof(earth_param_set)}(scalar_params | ||
| earth_param_set, | ||
| ) | ||
| ) | ||
| return CanopyModel() |
There was a problem hiding this comment.
CanopyModel: missing comma & wrong return
scalar_params earth_param_setlacks a comma – parsing fails.- The builder returns
CanopyModel()(recursive again). scalar_paramsis positional but never supplied by callers.
-parameters = Canopy.SharedCanopyParameters{FT, typeof(earth_param_set)}(scalar_params
- earth_param_set,
+parameters = Canopy.SharedCanopyParameters{FT, typeof(earth_param_set)}(
+ scalar_params,
+ earth_param_set,
)
…
-return CanopyModel()
+return Canopy.CanopyModel{FT}(; # or give the builder a new name
+ autotrophic_respiration,
+ radiative_transfer,
+ photosynthesis,
+ conductance,
+ hydraulics,
+ energy,
+ parameters,
+)Either expose a convenience wrapper or pass scalar_params from LandModel.
📝 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 CanopyModel(FT, domain, earth_param_set, scalar_params, forcing, prognostic_land_components; | |
| autotrophic_respiration = Canopy.AutotrophicRespirationModel{FT}(Canopy.AutotrophicRespirationParameters(FT)), | |
| radiative_transfer = Canopy.TwoStreamModel{FT}(Canopy.TwoStreamParameters(FT, domain, scalar_params)), | |
| photosynthesis = Canopy.FarquharModel{FT}(Canopy.FarquharParameters(FT, domain, scalar_params)), | |
| conductance = Canopy.MedlynConductanceModel{FT}(Canopy.MedlynConductanceParameters(FT, domain, scalar_params)), | |
| hydraulics = Canopy.PlantHydraulicsModel{FT}(Canopy.PlantHydraulicsParameters(scalar_params)), | |
| energy = Canopy.BigLeafEnergyModel{FT}(Canopy.BigLeafEnergyParameters{FT}(scalar_params)), | |
| parameters = Canopy.SharedCanopyParameters{FT, typeof(earth_param_set)}(scalar_params | |
| earth_param_set, | |
| ) | |
| ) | |
| return CanopyModel() | |
| function CanopyModel(FT, domain, earth_param_set, scalar_params, forcing, prognostic_land_components; | |
| autotrophic_respiration = Canopy.AutotrophicRespirationModel{FT}(Canopy.AutotrophicRespirationParameters(FT)), | |
| radiative_transfer = Canopy.TwoStreamModel{FT}(Canopy.TwoStreamParameters(FT, domain, scalar_params)), | |
| photosynthesis = Canopy.FarquharModel{FT}(Canopy.FarquharParameters(FT, domain, scalar_params)), | |
| conductance = Canopy.MedlynConductanceModel{FT}(Canopy.MedlynConductanceParameters(FT, domain, scalar_params)), | |
| hydraulics = Canopy.PlantHydraulicsModel{FT}(Canopy.PlantHydraulicsParameters(scalar_params)), | |
| energy = Canopy.BigLeafEnergyModel{FT}(Canopy.BigLeafEnergyParameters{FT}(scalar_params)), | |
| - parameters = Canopy.SharedCanopyParameters{FT, typeof(earth_param_set)}(scalar_params | |
| - earth_param_set, | |
| - ) | |
| + parameters = Canopy.SharedCanopyParameters{FT, typeof(earth_param_set)}( | |
| + scalar_params, | |
| + earth_param_set, | |
| + ) | |
| ) | |
| - return CanopyModel() | |
| + return Canopy.CanopyModel{FT}(; | |
| + autotrophic_respiration, | |
| + radiative_transfer, | |
| + photosynthesis, | |
| + conductance, | |
| + hydraulics, | |
| + energy, | |
| + parameters, | |
| + ) | |
| end |
🤖 Prompt for AI Agents
In src/simulations/model_setup.jl around lines 50 to 61, fix the missing comma
between scalar_params and earth_param_set in the parameters list to correct the
syntax error. Change the return statement from recursively calling CanopyModel()
with no arguments to returning the constructed CanopyModel instance with the
initialized fields. Also, ensure scalar_params is passed as an argument from the
caller, such as LandModel, or provide a wrapper function that supplies
scalar_params to avoid missing positional arguments.
| path = ClimaLand.Artifacts.soil_grids_params_artifact_path(; | ||
| lowres = true, | ||
| ClimaComms.context(subsurface_space), | ||
| ) | ||
| ) | ||
|
|
||
| S_s = | ||
| masked_to_value.( | ||
| ClimaCore.Fields.zeros(subsurface_space) .+ FT(1e-3), | ||
| soil_params_mask, | ||
| 1, | ||
| ) | ||
| Reads spatially varying parameters for the soil model, from NetCDF files | ||
| based on SoilGrids, | ||
| and regrids them to the grid defined by the | ||
| `subsurface_space` and `surface_space` of the Clima simulation, as appropriate. | ||
| Returns a NamedTuple of ClimaCore Fields. | ||
|
|
||
| soilgrids_artifact_path = | ||
| ClimaLand.Artifacts.soil_grids_params_artifact_path(; | ||
| lowres = true, | ||
| context, | ||
| ) | ||
| In particular, this file returns a field for | ||
| - various texture variables: volumetric fractions of organic matter, coarse fragments, and quartz. | ||
|
|
||
| The NetCDF files are stored in ClimaArtifacts and more detail on their origin | ||
| is provided there. The keyword arguments `regridder_type` and `extrapolation_bc` | ||
| affect the regridding by (1) changing how we interpolate to ClimaCore points which | ||
| are not in the data, and (2) changing how extrapolate to points beyond the range of the | ||
| data. | ||
|
|
||
| Any `path` can be provided, but this assumes that the parameters are stored under the names | ||
| `nu_ss_om`, `nu_ss_sand`, and `nu_ss_cf`. | ||
| """ | ||
| function soil_composition_parameters( | ||
| subsurface_space, | ||
| FT; | ||
| regridder_type = :InterpolationsRegridder, | ||
| extrapolation_bc = ( | ||
| Interpolations.Periodic(), | ||
| Interpolations.Flat(), | ||
| Interpolations.Flat(), | ||
| ), | ||
| path = ClimaLand.Artifacts.soil_grids_params_artifact_path(; | ||
| lowres = true, | ||
| ClimaComms.context(subsurface_space), | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Invalid keyword syntax in soil_composition_parameters default path
ClimaComms.context(subsurface_space) is passed without a keyword name, so Julia throws “keyword not assigned”.
-path = ClimaLand.Artifacts.soil_grids_params_artifact_path(;
- lowres = true,
- ClimaComms.context(subsurface_space),
- )
+path = ClimaLand.Artifacts.soil_grids_params_artifact_path(;
+ lowres = true,
+ context = ClimaComms.context(subsurface_space),
+)📝 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.
| path = ClimaLand.Artifacts.soil_grids_params_artifact_path(; | |
| lowres = true, | |
| ClimaComms.context(subsurface_space), | |
| ) | |
| ) | |
| S_s = | |
| masked_to_value.( | |
| ClimaCore.Fields.zeros(subsurface_space) .+ FT(1e-3), | |
| soil_params_mask, | |
| 1, | |
| ) | |
| Reads spatially varying parameters for the soil model, from NetCDF files | |
| based on SoilGrids, | |
| and regrids them to the grid defined by the | |
| `subsurface_space` and `surface_space` of the Clima simulation, as appropriate. | |
| Returns a NamedTuple of ClimaCore Fields. | |
| soilgrids_artifact_path = | |
| ClimaLand.Artifacts.soil_grids_params_artifact_path(; | |
| lowres = true, | |
| context, | |
| ) | |
| In particular, this file returns a field for | |
| - various texture variables: volumetric fractions of organic matter, coarse fragments, and quartz. | |
| The NetCDF files are stored in ClimaArtifacts and more detail on their origin | |
| is provided there. The keyword arguments `regridder_type` and `extrapolation_bc` | |
| affect the regridding by (1) changing how we interpolate to ClimaCore points which | |
| are not in the data, and (2) changing how extrapolate to points beyond the range of the | |
| data. | |
| Any `path` can be provided, but this assumes that the parameters are stored under the names | |
| `nu_ss_om`, `nu_ss_sand`, and `nu_ss_cf`. | |
| """ | |
| function soil_composition_parameters( | |
| subsurface_space, | |
| FT; | |
| regridder_type = :InterpolationsRegridder, | |
| extrapolation_bc = ( | |
| Interpolations.Periodic(), | |
| Interpolations.Flat(), | |
| Interpolations.Flat(), | |
| ), | |
| path = ClimaLand.Artifacts.soil_grids_params_artifact_path(; | |
| lowres = true, | |
| ClimaComms.context(subsurface_space), | |
| ) | |
| ) | |
| function soil_composition_parameters( | |
| subsurface_space, | |
| FT; | |
| regridder_type = :InterpolationsRegridder, | |
| extrapolation_bc = ( | |
| Interpolations.Periodic(), | |
| Interpolations.Flat(), | |
| Interpolations.Flat(), | |
| ), | |
| path = ClimaLand.Artifacts.soil_grids_params_artifact_path(; | |
| lowres = true, | |
| context = ClimaComms.context(subsurface_space), | |
| ) | |
| ) | |
| # ... | |
| end |
🤖 Prompt for AI Agents
In src/simulations/spatial_parameters.jl around lines 540 to 577, the default
argument for `path` in the `soil_composition_parameters` function incorrectly
passes `ClimaComms.context(subsurface_space)` without a keyword name, causing a
Julia syntax error. Fix this by assigning a keyword name to
`ClimaComms.context(subsurface_space)` in the argument list, for example, prefix
it with the appropriate keyword like `context =
ClimaComms.context(subsurface_space)`.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
src/simulations/model_setup.jl (7)
4-27:⚠️ Potential issueFix function shadowing and return statement
The function name shadows the struct type, causing infinite recursion. Also fix type consistency:
-function EnergyHydrology(FT, domain, earth_param_set, forcing, prognostic_land_components; +function build_energy_hydrology(FT, domain, earth_param_set, forcing, prognostic_land_components; ... - S_s = ClimaCore.Fields.zeros(domain.space.subsurface) .+ 1e-3, + S_s = ClimaCore.Fields.zeros(domain.space.subsurface) .+ FT(1e-3), ... - sources = (RootExtraction{FT}(), Soil.PhaseChange{FT}()) + sources = (Soil.RootExtraction{FT}(), Soil.PhaseChange{FT}()) ... - return EnergyHydrology{FT}(; parameters, domain, boundary_conditions, sources) + return ClimaLand.Soil.EnergyHydrology{FT}(; parameters, domain, boundary_conditions, sources)
29-37:⚠️ Potential issueFix function shadowing
-function SnowModel(FT, domain, earth_param_set, forcing, prognostic_land_components, Δt; +function build_snow_model(FT, domain, earth_param_set, forcing, prognostic_land_components, Δt; ... - return SnowModel{FT}(; boundary_conditions, domain, parameters) + return ClimaLand.Snow.SnowModel{FT}(; boundary_conditions, domain, parameters)
39-51:⚠️ Potential issueFix multiple critical issues
-function SoilCO2Model(FT, domain, earth_param_set, forcing, prognostic_land_components, soil_params; +function build_soilco2_model(FT, domain, earth_param_set, forcing, prognostic_land_components, soil_params; ... - bottom_bc = Soil.Biogeochemistry.SoilCO2FluxBC((p, t) -> 0.0) + bottom_bc = Soil.Biogeochemistry.SoilCO2FluxBC((p, t) -> zero(FT)) - drivers = Soil.Biogeochemistry.SoilDrivers(Soil.Biogeochemistry.PrognosticMet(soil_params), + drivers = Soil.Biogeochemistry.SoilDrivers(Soil.Biogeochemistry.PrognosticMet(soil_params), ... - (; top = top_bc, bottom = bot_bc) + (; top = top_bc, bottom = bottom_bc) - return SoilCO2Model{FT}(; boundary_conditions, sources, domain, parameters, drivers) + sources = () # Define sources or remove from return + return ClimaLand.Soil.Biogeochemistry.SoilCO2Model{FT}(; boundary_conditions, domain, parameters, drivers)
52-65:⚠️ Potential issueFix syntax errors and return statement
function CanopyModel(FT, domain, earth_param_set, forcing, prognostic_land_components; - scalar_params = ; + scalar_params, # Make required parameter ... - parameters = Canopy.SharedCanopyParameters{FT, typeof(earth_param_set)}(scalar_params - earth_param_set, + parameters = Canopy.SharedCanopyParameters{FT, typeof(earth_param_set)}(scalar_params, + earth_param_set, ) ) - return CanopyModel() + return ClimaLand.Canopy.CanopyModel{FT}(; + autotrophic_respiration, + radiative_transfer, + photosynthesis, + conductance, + hydraulics, + energy, + parameters, + domain, + boundary_conditions = Canopy.AtmosDrivenCanopyBC(forcing.atmos, forcing.radiation, prognostic_land_components) + )
88-93:⚠️ Potential issueFix type consistency and undefined variables
- soil_organic_carbon = ClimaLand.PrescribedSoilOrganicCarbon{FT}(TimeVaryingInput((t) -> 5)), + soil_organic_carbon = ClimaLand.PrescribedSoilOrganicCarbon{FT}(TimeVaryingInput(t -> FT(5))), soil = EnergyHydrology(FT, domain, earth_param_set, forcing, prognostic_land_components), - canopy = CanopyModel(FT, domain, earth_param_set, forcing, prognostic_land_components) + canopy = CanopyModel(FT, domain, earth_param_set, forcing, prognostic_land_components), snow = SnowModel(FT, domain, earth_param_set, forcing, prognostic_land_components, Δt), - soilco2 = SoilCO2Model(FT, domain, earth_param_set, merge(forcing, (;soil_organic_carbon = soil_organic_carbon)), prognostic_land_components, soil_params), + soilco2 = SoilCO2Model(FT, domain, earth_param_set, merge(forcing, (;soil_organic_carbon = soil_organic_carbon)), prognostic_land_components, soil.parameters),
98-105:⚠️ Potential issueFix typo and type comparison
- @assert progonostic_land_components == (:canopy, :snow, :soil, :soilco2) + @assert prognostic_land_components == (:canopy, :snow, :soil, :soilco2) ... - @assert soilco2.boundary_conditions.top isa AtmosCO2StateBC() + @assert soilco2.boundary_conditions.top isa Soil.Biogeochemistry.AtmosCO2StateBC
90-90: 💡 Verification agent🧩 Analysis chain
Missing scalar_params for CanopyModel
The
CanopyModelfunction requiresscalar_paramsbut it's not provided.
🏁 Script executed:
#!/bin/bash # Check if scalar_params is defined elsewhere or if there's a default rg -A 5 "scalar_params" --type juliaLength of output: 1577
Define and Pass
scalar_paramsinto CanopyModelThe call to
CanopyModelis missing itsscalar_paramsargument—scalar_params = ;is uninitialized and never passed through.• File:
src/simulations/model_setup.jlaround line 90
• Initializescalar_paramsbefore use (for example, with the appropriateCanopy.ScalarParameters(...))
• Update the constructor call to includescalar_paramsSuggested diff:
- scalar_params = ; + scalar_params = Canopy.ScalarParameters(FT, domain, earth_param_set) # ← fill in with the correct parameters - canopy = CanopyModel(FT, domain, earth_param_set, forcing, prognostic_land_components) + canopy = CanopyModel(FT, domain, earth_param_set, forcing, prognostic_land_components, scalar_params)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/shared_utilities/drivers.jl(1 hunks)src/simulations/model_setup.jl(1 hunks)
🔇 Additional comments (1)
src/shared_utilities/drivers.jl (1)
1392-1392: LGTM!The change to named tuple improves clarity and consistency with the model setup interface.
| @assert canopy.boundary_conditions.ground == PrognosticGroundConditions() | ||
|
|
||
| # Make sure that the soil knows that the canopy is present | ||
| @assert RootExtraction{FT}() ∈ soil.sources |
There was a problem hiding this comment.
Add missing namespace prefixes
- @assert canopy.boundary_conditions.ground == PrognosticGroundConditions()
+ @assert canopy.boundary_conditions.ground == Canopy.PrognosticGroundConditions()
...
- @assert RootExtraction{FT}() ∈ soil.sources
+ @assert Soil.RootExtraction{FT}() ∈ soil.sources📝 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.
| @assert canopy.boundary_conditions.ground == PrognosticGroundConditions() | |
| # Make sure that the soil knows that the canopy is present | |
| @assert RootExtraction{FT}() ∈ soil.sources | |
| @assert canopy.boundary_conditions.ground == Canopy.PrognosticGroundConditions() | |
| # Make sure that the soil knows that the canopy is present | |
| @assert Soil.RootExtraction{FT}() ∈ soil.sources |
🤖 Prompt for AI Agents
In src/simulations/model_setup.jl around lines 118 to 121, the code uses types
or functions like PrognosticGroundConditions and RootExtraction without their
required namespace prefixes. To fix this, identify the correct modules or
namespaces these belong to and prefix them accordingly to avoid undefined or
ambiguous references. Update the code to use fully qualified names for these
types or functions.
Purpose
Partially addresses Issue #795
To-do
[X] Merge PR #1141
[X] Merge PR #1151
[X] Merge PR #1150
[X] Merge PR #1152
[X] Rebase this
[ ] clean up the output of era5_forcing function (return a struct?)
[X] Split soil parameters up
[ ] Split canopy parameters up and make those constructors
[ ] move soil albedo to its own thing in top BC
Content