Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Global run of complex land model with ERA5 forcing #591

Merged
merged 1 commit into from
Jun 21, 2024

Conversation

kmdeck
Copy link
Collaborator

@kmdeck kmdeck commented Apr 25, 2024

Purpose

Carry out global run of soil+canopy model on GPU and CPU

To-do

Content

  1. Previously, we had only ever simulated Richards model globally. Because the soil properties vary spatially, certain functions that are only used by EnergyHydrology needed to be updated to be broadcastable. The primary change is because the parameter struct contains fields, but is not a field of a parameter structs. this means that functions that we broadcasted over a Ref of the parameter struct before now must be supplied each parameter field as an argument. This led to a LOT of changes (volumetric_heat_capacity, volumetric_internal_energy, volumetric_internal_energy_liq, phase_change_source, soil_resistance)
  2. Parameters that were marked as FT in the EnergyHydrologyParameter struct now are a unionof FT and Field. Some parameters are only defined on the surface, rather than throughout the soil, so they need a different parametric type
  3. added benchmark - on GPU
  4. added experiment - GPU/CPU

Note: For the benchmark - currently about 1/4 of the time is in setup rather than solve, so maybe we need to change the setup of the simulation. however, making it longer leads to failures, so Im not sure yet.

Note: The experiment does not run on GPU in CI - on caltech - , but it does on GPU on clima (benchmark). For now I removed the run on caltech


  • I have read and checked the items on the review checklist.

@kmdeck kmdeck added the Run benchmarks Add this label to run benchmarks on clima label Jun 19, 2024
@kmdeck kmdeck marked this pull request as ready for review June 19, 2024 00:06
@kmdeck kmdeck self-assigned this Jun 19, 2024
Copy link
Member

@juliasloan25 juliasloan25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is awesome, thank you Kat! It would be nice to reduce some of the code duplication between the land.jl and global_soil_canopy.jl drivers (and maybe for the other benchmark runs too), but this is fine for now :)

src/integrated/soil_canopy_model.jl Show resolved Hide resolved
src/standalone/Vegetation/PlantHydraulics.jl Show resolved Hide resolved
ext/CreateParametersExt.jl Show resolved Hide resolved
.buildkite/target/pipeline.yml Show resolved Hide resolved
experiments/integrated/global/global_soil_canopy.jl Outdated Show resolved Hide resolved
experiments/benchmarks/land.jl Show resolved Hide resolved
experiments/benchmarks/land.jl Show resolved Hide resolved
experiments/benchmarks/land.jl Outdated Show resolved Hide resolved
experiments/integrated/global/global_soil_canopy.jl Outdated Show resolved Hide resolved
Copy link
Member

@juliasloan25 juliasloan25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thank you for addressing the review so thoroughly :) I left a few more minor comments

@kmdeck
Copy link
Collaborator Author

kmdeck commented Jun 21, 2024

LGTM! Thank you for addressing the review so thoroughly :) I left a few more minor comments

thank you for the quick and thorough review!

@kmdeck kmdeck enabled auto-merge June 21, 2024 17:38
SAI = FT(0.0) # m2/m2
f_root_to_shoot = FT(3.5)
RAI = FT(1.0)
K_sat_plant = 5e-9 # m/s # seems much too small?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
K_sat_plant = 5e-9 # m/s # seems much too small?
K_sat_plant = FT(5e-9) # m/s # seems much too small?

Comment on lines +367 to +369
h_stem = 0.0
h_leaf = 1.0
zmax = 0.0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there FT missing here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

youre right! I will go through the file and update it in this PR #669


# Set the soil CO2 BC to being atmospheric CO2
soilco2_top_bc = Soil.Biogeochemistry.AtmosCO2StateBC()
soilco2_bot_bc = Soil.Biogeochemistry.SoilCO2FluxBC((p, t) -> 0.0) # no flux
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's probably a FT missing around 0.0

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we convert the BC into the FT type internally here:

return FT.(bc.bc(p, t)) .+ FT.(ClimaCore.Fields.zeros(axes(Δz)))

(these are very allocation heavy and we need to update this code anyways!)


init_soil(ν, θ_r) = θ_r + (ν - θ_r) / 2
Y.soil.ϑ_l .= init_soil.(ν, θ_r)
Y.soil.θ_i .= FT(0.0)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typically, you don't need FT when you .= into a Field

@@ -321,7 +321,7 @@ function ClimaLand.Canopy.set_canopy_prescribed_field!(
t,
) where {FT}
(; LAIfunction, SAI, RAI) = component.parameters.ai_parameterization
evaluate!(p.canopy.hydraulics.area_index.leaf, LAIfunction, t)
evaluate!(p.canopy.hydraulics.area_index.leaf, LAIfunction, floor(t))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe using round is better?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Youre right, but I think LAI data is updated every 8 days, so floor vs round makes no real difference here. I am leaving this as is since we will remove it entirely when the bug fix is in.

@kmdeck kmdeck merged commit a2bfc53 into main Jun 21, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request LSMv1 Run benchmarks Add this label to run benchmarks on clima
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants