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

Add SoilCanopy diagnostics #699

Merged
merged 1 commit into from
Aug 17, 2024
Merged

Add SoilCanopy diagnostics #699

merged 1 commit into from
Aug 17, 2024

Conversation

AlexisRenchon
Copy link
Member

@AlexisRenchon AlexisRenchon commented Jul 16, 2024

Purpose

This is a second major PR to introduce diagnostics in ClimaLand.

To do:

  • Update SoilCanopyModel weekly long run with :long output_vars - test if it works!
  • docs: optional kwarg output_vars = :short or :long --> write it so it is called and written in docs, output_period = :hour or :month or ...
  • add diagnostics to experiments/integrated/global/global_soil_canopy.jl, plots like Bucket, make sure it is grabbed by buildkite
  • macro to reduce code duplication in compute_ and add_diag ( ? )
  • fix names, units, comments of SoilCanopyModel diagnostics variables
  • add more default methods, period of averaging, what variables, etc.
  • Update where variable are stored if needed, add SIF

next PR 1:

  • Add @has_error macro
  • Somewhere, say which variable is 2D and which one is 3D (i.e., which has depth)
  • 2nd round of names, units, comment etc. review, address the one still WIP in this PR (that need further discussion with Renato and Kat)
  • Kat: we also have some runoff variables, but where or not they are present depends on the type of RunoffModel we choose. maybe this is something we can accomodate? E.g. NoRunoff() has no additional variables, while TopModel has subsurface, surface runoff in p.soil
  • Make sure carbon flux units are consistent, and chose: mol co2 m-2 s-1? Co2 or c? mol or kg?

next PR 2:

  • more tests
  • better docs
  • use diagnostics in all files (need to update experiments, tests, etc.)
  • Add land-sea mask as output by default, might be in the domain? (where to compute it? --> ask Kat)

next PR 3:

@AlexisRenchon AlexisRenchon force-pushed the ar/diagnostics_update branch from 303010d to 3965bc3 Compare July 29, 2024 18:32
@AlexisRenchon AlexisRenchon marked this pull request as draft July 29, 2024 18:33
@AlexisRenchon AlexisRenchon force-pushed the ar/diagnostics_update branch from 2f4488a to eb00eba Compare August 5, 2024 16:29
@AlexisRenchon AlexisRenchon requested a review from kmdeck August 5, 2024 21:43
@AlexisRenchon
Copy link
Member Author

AlexisRenchon commented Aug 5, 2024

@kmdeck
@braghiere

Could you just look at:
land_compute_methods.jl - do I have everything we compute in p and Y? Does the code organization look good?

define_diagnostics.jl - Are the names, units and comments good? You can start at line 182 (SoilCanopyModel)

@AlexisRenchon
Copy link
Member Author

@kmdeck @Sbozzolo

Should we add a test that check if all variables of a model (e.g., SoilCanopyModel) are defined in the diagnostics?
That would be good to ensure in CI that if we add a new variable, it has to be in diagnostics

@Sbozzolo
Copy link
Member

Sbozzolo commented Aug 6, 2024

@kmdeck @Sbozzolo

Should we add a test that check if all variables of a model (e.g., SoilCanopyModel) are defined in the diagnostics? That would be good to ensure in CI that if we add a new variable, it has to be in diagnostics

There is no requirement for variables defined to be in the model to be in the diagnostics too: only those that are scientifically interesting

@land_compute "soil_aerodynamic_resistance" SoilCanopyModel p.soil.turbulent_fluxes.r_ae
@land_compute "soil_latent_heat_flux" SoilCanopyModel p.soil.turbulent_flux.lhf
@land_compute "soil_sensible_heat_flux" SoilCanopyModel p.soil.turbulent_fluxes.shf
@land_compute "vapor_flux" SoilCanopyModel p.soil.turbulent_fluxes.vapor_flux
Copy link
Member

Choose a reason for hiding this comment

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

we also have some runoff variables, but where or not they are present depends on the type of RunoffModel we choose. maybe this is something we can accomodate? E.g. NoRunoff() has no additional variables, while TopModel has subsurface, surface runoff in p.soil

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is fine - user decide which diagnostic variable to get, so we can put runoff here, and then user can grab it or not...
For the default, I will think about how to handle this, I am not sure

Artifacts.toml Outdated Show resolved Hide resolved
short_name = "ra",
long_name = "Autotrophic Respiration",
standard_name = "autotrophic_respiration",
units = "mol CO2 m^-2 s^-1", # note throughout: make sure what we express as kg or mol, CO2 or C
Copy link
Member

Choose a reason for hiding this comment

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

I think this may be kg of C.
I think we have a bug in the units in the code. Maybe worth fixing in this PR?
https://github.com/CliMA/ClimaLand.jl/blob/main/src/standalone/Vegetation/canopy_parameterizations.jl#L1004 [maintenance is in kg/m^2/s]
but here it is used with An: https://github.com/CliMA/ClimaLand.jl/blob/main/src/standalone/Vegetation/canopy_parameterizations.jl#L1040
and An has units of mol CO2.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it would be good to decide together what to do with flux of CO2 @braghiere , do we express them is mol CO2, kg C, or other?
These are all SI, so we just have to pick one.
Usually, in the literature, we use mol CO2 for fluxes (umol), and kg C for budgets (change in pool size or pool size).
I would vote for mol CO2, but let's all agree.

Copy link
Member

Choose a reason for hiding this comment

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

Yep. Carbon fluxes is usually in umolCO2.m-2.s-1 (or kgCm-2-day-1) and pools are in kgC.

@AlexisRenchon AlexisRenchon force-pushed the ar/diagnostics_update branch from 40fcc0f to b409e1a Compare August 7, 2024 16:22
@AlexisRenchon AlexisRenchon force-pushed the ar/diagnostics_update branch from a538e04 to f19144f Compare August 16, 2024 21:33
@AlexisRenchon AlexisRenchon marked this pull request as ready for review August 16, 2024 21:35
@AlexisRenchon AlexisRenchon force-pushed the ar/diagnostics_update branch 2 times, most recently from 5bb93ea to e9b3e15 Compare August 16, 2024 21:39
@AlexisRenchon AlexisRenchon force-pushed the ar/diagnostics_update branch 5 times, most recently from 123ae0c to 775c321 Compare August 16, 2024 23:01
@AlexisRenchon AlexisRenchon requested a review from Sbozzolo August 16, 2024 23:28
@Sbozzolo Sbozzolo force-pushed the ar/diagnostics_update branch 3 times, most recently from d7446fa to 0b76a1b Compare August 17, 2024 05:47
@AlexisRenchon AlexisRenchon force-pushed the ar/diagnostics_update branch from 0b76a1b to ae13b0d Compare August 17, 2024 14:43
@Sbozzolo Sbozzolo force-pushed the ar/diagnostics_update branch 4 times, most recently from 01c3183 to 9f4c20f Compare August 17, 2024 15:17
@AlexisRenchon AlexisRenchon force-pushed the ar/diagnostics_update branch 2 times, most recently from 0fd14bf to e700a9d Compare August 17, 2024 16:11
@Sbozzolo Sbozzolo self-requested a review August 17, 2024 16:51
This PR finalizes the addition of SoilCanopyModel diagnostics.
It also adds a macro to generate land compute functions,
an optional argument for period averaging for default diagnostics,
an optional argument for ":long" (all) or ":short" (a few essentials)
for default diagnostics.
Diagnostics are now used in global_soil_canopy.jl.
EnergyHydrology model diagnostics_compute methods have been added.
Plots are now prettier, using GeoMakie.

Co-authored-by: AlexisRenchon <a.renchon@gmail.com>
Co-authored-by: SBozzolo <gbozzola@caltech.edu>
Co-authored-by: kmdeck <kdeck@caltech.edu>
Co-authored-by: braghiere <renato.braghiere@gmail.com>
@AlexisRenchon AlexisRenchon force-pushed the ar/diagnostics_update branch from e700a9d to 9998ef0 Compare August 17, 2024 17:30
@AlexisRenchon AlexisRenchon merged commit ffdef8b into main Aug 17, 2024
9 of 10 checks passed
@AlexisRenchon AlexisRenchon deleted the ar/diagnostics_update branch October 21, 2024 18:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants