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

integrate neural network into Snow.jl architecture #905

Closed
wants to merge 29 commits into from

Conversation

a-charbon
Copy link
Contributor

Purpose

Integrate the neural snow depth model into the Snow model architecture. This involves small changes to Snow.jl and SnowParameterizations.jl as well as expanding the NeuralSnow extension to allow the neural model as a type of AbstractDensityModel. "CSV" was also added to the buildkite dependencies to permit testing of the NeuralSnow extension, and BSON has been appropriately added to the ClimaLand weak dependencies to enable the expanded functionality of the extension.

To-do

Questions are commented in the changed files about different possible approaches. With this PR merged, I can also add a slightly faster and more abstract/streamlined form of the neural model itself (changes to ModelTools, etc).

Content

Expanded NeuralSnow extension and changed some of the syntax of the Snow model.


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

@a-charbon a-charbon requested a review from kmdeck November 1, 2024 00:30
juliasloan25 and others added 5 commits November 1, 2024 09:44
use updated Bonan comparison artifact
* Change soil albedo to depend of soil moisture

PAR and NIR albedo are moved from the EnergyHydrologyParameters
to the auxiliary vars via AtmosDrivenFluxBC. The parameters now
hold PAR/NIR wet and dry as a field of float. The old method
of creating the parameters with PAR/NIR albedo is marked as deprecated,
and the constructor just sets both the wet and dry values to the
passed albedo.

* Use relative saturation

* Make relative saturation in albedo calc not include ice

* Add soil albedos to diagnostics

* Make experiments use ground albedo maps

* Improve diagnostic var names

* Update docstring for PrognosticSoilConditions

* Make update_albedo! use effective_saturation

* Combine par and nir albedo diagnostics

* Refactor soil albedo calculation

update_albedo is no longer called conditionally. Now it is dispatched
to a function that either updates albedo or a function that does nothing.

update_albedo takes in model.params instead of each param individually.

update_albedo now just uses the top layer of if delta_z is larger than
the top depth.

top_depth is now a parameter instead of hard coded

cache var names are changed. Physically irrelevent vars changed to scratch vars

Delete integrated soil-canopy test

Previosuly, PAR and NIR albedo were held in a PrognosticSoilConditions
struct, which acted as an intermediary between the soil and canopy models.

Now, PAR and NIR albedo are stored in the cache, and the struct is only
used for dispatch.

The integrated soil-canopy test created a PrognosticSoilConditions
struct and filled in with a PAR and NIR albedo. Then it called
ground_albedo_PAR and ground_albedo_NIR to check the values.

Both of those functions now read from the cache. In order to test them, a
SoilCanopy model would need to be created, initialized, and then updated.
This seems to complex for a test, so this test is deleted.

Make run_fluxnet work with new SoilEnergy

Make update_albedo! comments better

Apply suggestions from code review

Co-authored-by: Gabriele Bozzola <gbozzola@caltech.edu>

Change top_depth name to alebdo_calc_top_thickness

Add convenience constructor for soil albedo maps

Fix calls to create_soil_albedo_vars

Fix spelling errors

Simplify and correct calculate_albedo!

Add citation and fix albedo variable constructor error

* Remake soil_canopy_lsm test

This test was deleted because it would not function with previous changes.
As requested, this test was recreated to work with the changes. The previous
test simply set an albedo it the top bc struct and then checked it using
the ground_albedo functions. With the changes, this would only work with an
initialized model. Because this is an integrated model test, I thought it
made sense to test the SoilCanopy model instead of just creating a
Soil model and checking if the function works. This test, like the
old version, calls the ground_albedo functions and checks their value.
This does seem very involved for a test, so maybe it should just create
a EnergyHydrology model and call the function on its Y and p

* Make suggested changes from Gabriele

change create_soil_albedo_vars to clm_soil_albedo_properties
and use map in it to reduce code duplication

made stylistic change when taking mean of PAR and NIR albedo

change <:FT to just FT

Move salb to short diagnostics

Also make other requested changes such as changing
surface_liquid_fraction to surface_eff_sat and making docstring more
specific

Fix albedo constructor and change benchmark sampling

Move salb disgnostic and reduce profiling samples

revert samples

* Merge soil param convenience constructors

Fix merge issue

fix merge issue 2

* Fix albedo params in snowy_land.jl
Add long_run global diagnostics folder to gitignore

Edit gitignore
src/standalone/Snow/Snow.jl Outdated Show resolved Hide resolved
src/standalone/Snow/Snow.jl Outdated Show resolved Hide resolved
src/standalone/Snow/Snow.jl Outdated Show resolved Hide resolved
src/standalone/Snow/Snow.jl Outdated Show resolved Hide resolved
src/standalone/Snow/Snow.jl Outdated Show resolved Hide resolved
src/standalone/Snow/Snow.jl Outdated Show resolved Hide resolved
ModelTools.settimescale!(zmodel, 86400.0)
return zmodel

#should I write this function to give the ability to also load from local file too?
Copy link
Member

Choose a reason for hiding this comment

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

our style is more to have the download link download the file, managed by ClimaArtifacts, and then to load from file.

Perhaps you could switch to that style (read from local file), and open an issue in ClimaArtifacts to add this? we can resolve that later, though


end

#do tests need to be made for these functions? since it is an extension?
Copy link
Member

Choose a reason for hiding this comment

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

we have neural snow model extension tests currently:
https://github.com/CliMA/ClimaLand.jl/blob/main/test/standalone/Snow/tool_tests.jl

it would be good to add some tests for these functions

kmdeck and others added 17 commits November 14, 2024 13:54
* Fix warnings in tests

Replace deprecated reference_date arg

The ClimaUtilities DataHandler, TimeVaryingInput, and EveryCalendarDtSchedule
function were called with reference_date as an argument, which is
deprecated. This commit replaces them with start_date, and replaces
other uses of reference_date as an arg to functions that pass the arg
to EveryCalendarDtSchedule.

Fix warning in test for unused type declaration

Update common_diagnostics example in docs

Annotate start date

* Bump ClimaUtilities compat

ClimaUtilities compat bumped from 0.1.16 to 0.1.17.
This allows call to detect_restart_file to allways use
the non-deprecated argument for style.
* change artifact to 2008 and less freq LAI
use ClimaArtifacts Lehmann 2008 evaporation artifact
@kmdeck
Copy link
Member

kmdeck commented Dec 6, 2024

moved to #941

@kmdeck kmdeck closed this Dec 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants