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

Bucket diagnostics #628

Merged
merged 2 commits into from
Jun 26, 2024
Merged

Bucket diagnostics #628

merged 2 commits into from
Jun 26, 2024

Conversation

AlexisRenchon
Copy link
Member

@AlexisRenchon AlexisRenchon commented May 24, 2024

Purpose

This PR adds diagnostics to ClimaLand, as a new module in /src.
This module is built on ClimaDiagnostics.jl, which is also used by
ClimaAtmos. It contains default diagnostics methods for different
ClimaLand models (for now, BucketModel and SoilCanopyModel), those
diagnostics contains metadata such as where diagnostics variables are
stored in those models, what is there names (short, long, standard)
and physical units, with additional comments. It creates a Dict of those
variables, and also default Scheduling methods, such as hourly average
or monthly maximum. Users can define their own diagnostic variables or
scheduling, and developpers can add defaults. More information are
available in the documentation.

@AlexisRenchon AlexisRenchon added the enhancement New feature or request label May 24, 2024
@AlexisRenchon AlexisRenchon self-assigned this May 24, 2024
@AlexisRenchon AlexisRenchon force-pushed the ar/bucket_diagnostics branch 2 times, most recently from b94eca3 to 6478bb0 Compare May 31, 2024 16:53
@AlexisRenchon
Copy link
Member Author

@Sbozzolo

Do you know why ClimaLandSimulations CI fail?
The error is

Run julia --project= -e 'using Pkg; Pkg.develop(path="$(pwd())"); Pkg.develop(path="$(pwd())/lib/ClimaLandSimulations")'
   Resolving package versions...
ERROR: Unsatisfiable requirements detected for package ClimaDiagnostics [1ecacbb8]:
 ClimaDiagnostics [1ecacbb8] log:
 ├─possible versions are: 0.0.1-0.0.5 or uninstalled
 └─restricted to versions 0.2 by ClimaLand [08f[4](https://github.com/CliMA/ClimaLand.jl/actions/runs/9321596680/job/25660930914?pr=628#step:5:5)d4ce] — no versions left
   └─ClimaLand [08f4d4ce] log:
     ├─possible versions are: 0.12.2 or uninstalled
     └─ClimaLand [08f4d4ce] is fixed to version 0.12.2

But if I run

julia --project= -e 'using Pkg; Pkg.develop(path="$(pwd())"); Pkg.develop(path="$(pwd())/lib/ClimaLandSimulations")'

locally, it works just fine

@Sbozzolo
Copy link
Member

Maybe you have to add ClimaDiagnostics to the Project.toml for ClimaLandSimulations?

@AlexisRenchon AlexisRenchon force-pushed the ar/bucket_diagnostics branch 2 times, most recently from d76adf3 to 044171a Compare June 14, 2024 19:50
@AlexisRenchon
Copy link
Member Author

@kmdeck Could you just look at src/Diagnostics/define_diagnostics.jl and check if the names, units, and comments look good? No need to review the full PR!

Thank you!!

@AlexisRenchon
Copy link
Member Author

@Sbozzolo

  • I moved the experiment to test (there is nothing new in experiment now). The idea is having a global bucket model in test that then calls some Diagnostic functions and test them. To do that, I had to add SciMLBase and ClimaTimeSteppers to the test Project.toml, I am not sure if this is something we are trying to avoid.
  • I added comments and changed some names to src/Diagnostics/define_diagnostics.jl
  • I added your macro to generate error functions
  • Docs: available_diagnostics works well
  • Docs: I added more content to developers and users documentation page, but there is still more to add (and add more links)
  • All CI checks passed

@@ -19,13 +19,13 @@ jobs:
- uses: julia-actions/setup-julia@latest
with:
version: '1.10'
- uses: julia-actions/cache@v2
# - uses: julia-actions/cache@v2
Copy link
Member

Choose a reason for hiding this comment

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

You can try uncommenting this

Copy link
Member Author

Choose a reason for hiding this comment

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

Let's go back to this at some point, as discussed, this still doesn't work

Copy link
Member

Choose a reason for hiding this comment

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

Maybe leave a comment on why this is commented out

docs/src/diagnostics/developers_diagnostics.md Outdated Show resolved Hide resolved
docs/src/diagnostics/developers_diagnostics.md Outdated Show resolved Hide resolved
error_diagnostic_variable(variable, land_model::T) where {T} =
error("Cannot compute $variable with model = $T")

macro generate_error_functions(function_names...)
Copy link
Member

Choose a reason for hiding this comment

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

Did this work?

Let's add tests to check that it works and let's add comments to explain how this works.

Copy link
Member Author

Choose a reason for hiding this comment

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

the test didn't work, let's discuss this when we meet

@Sbozzolo
Copy link
Member

@Sbozzolo

  • I moved the experiment to test (there is nothing new in experiment now). The idea is having a global bucket model in test that then calls some Diagnostic functions and test them. To do that, I had to add SciMLBase and ClimaTimeSteppers to the test Project.toml, I am not sure if this is something we are trying to avoid.

We should try to have more isolated unit tests that test the various functions, instead of a full experiment.

Artifacts.toml Outdated
Comment on lines 4 to 5
[land_albedo]
git-tree-sha1 = "5f7141a13994d5b334f1240e1bd5474ad44d96c4"
Copy link
Member

Choose a reason for hiding this comment

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

This should probably not be here

Return a `DiagnosticVariable` from its `short_name`, if it exists.
"""
function get_diagnostic_variable(short_name)
haskey(ALL_DIAGNOSTICS, short_name) ||
Copy link
Member

Choose a reason for hiding this comment

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

For your reference, as discussed

@test_throws ErrorException get_diagnostic_variable("bob")

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 added that test, BUT, currently we do not call define_diagnostics! inside src (only in docs), so get_diagnostic_variable would throw an error with any variable...

Copy link
Member

Choose a reason for hiding this comment

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

You can call define_diagnostics in your test

end

@generate_error_functions "albedo" "net_radiation" "surface_temperature" "surface_specific_humidity" "latent_heat_flux" "aerodynamic_resistance" "sensible_heat_flux" "vapor_flux" "surface_air_density" "soil_temperature" "subsurface_water_storage" "surface_water_content" "snow_water_equivalent"

Copy link
Member

@Sbozzolo Sbozzolo Jun 14, 2024

Choose a reason for hiding this comment

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

To test this

@test_throws ErrorException("cannot albedo with nothing") compute_albedo!(_, _, _, _, nothing)

Also check that compute_albedo! is defined

Copy link
Member Author

Choose a reason for hiding this comment

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

julia> using Test

julia> using ClimaLand

julia> @test isdefined(ClimaLand.Diagnostics, :compute_albedo!)
Test Passed

julia> @test_throws ErrorException("cannot albedo with nothing") ClimaLand.Diagnostics.comput
e_albedo!(_, _, _, _, nothing)
ERROR: syntax: all-underscore identifier used as rvalue around REPL[9]:1
Stacktrace:
 [1] top-level scope
   @ REPL[9]:1

```

Your diagnostics have now been written in netcdf files in your output folder.

Copy link
Member

Choose a reason for hiding this comment

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

What if I want to compute some existing diagnostics but with different Writers? Or with different frequencies?

docs/make.jl Outdated Show resolved Hide resolved
src/ClimaLand.jl Outdated Show resolved Hide resolved
@AlexisRenchon
Copy link
Member Author

Let's discuss the Bucket experiment a bit, we want to use an existing experiment, but it would be good if variables vary in space at least... I am not sure it is the case with global_bucket_function.jl (our heatmap plots are 1 color), maybe we should use another. We were using global_bucket_temporalmap.jl because of this, which has albedo varying in space (read from a file), but it defines the bucket model inside a function that I commented out before... and we want to not change the previous code but add diagnostics to it. Anyway, let's discuss.

@Sbozzolo
Copy link
Member

Let's not forget about the issue on @generate_error_functions

@AlexisRenchon AlexisRenchon marked this pull request as ready for review June 26, 2024 16:51
Copy link
Member

@Sbozzolo Sbozzolo left a comment

Choose a reason for hiding this comment

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

Some work is left for a future PR, but this looks good!

Thank you very much!

@Sbozzolo Sbozzolo force-pushed the ar/bucket_diagnostics branch 6 times, most recently from 95b0f07 to daa9ded Compare June 26, 2024 19:09
AlexisRenchon and others added 2 commits June 26, 2024 12:23
This PR adds diagnostics to ClimaLand, as a new module in /src.
This module is built on ClimaDiagnostics.jl, which is also used by
ClimaAtmos. It contains default diagnostics methods for different
ClimaLand models (for now, BucketModel and SoilCanopyModel), those
diagnostics contains metadata such as where diagnostics variables are
stored in those models, what is there names (short, long, standard)
and physical units, with additional comments. It creates a Dict of those
variables, and also default Scheduling methods, such as hourly average
or monthly maximum. Users can define their own diagnostic variables or
scheduling, and developers can add defaults. More information is
available in the documentation.
The tests were only comparing the albedo function job
@AlexisRenchon AlexisRenchon merged commit b26dfd8 into main Jun 26, 2024
9 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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants