Feature/add maximum snow albedo option #835
Conversation
…store with the lake options error message
| @@ -2,7 +2,8 @@ | |||
| from vic.vic import ffi | |||
| from vic import lib as vic_lib | |||
|
|
|||
|
|
|||
| # TODO: design new test to not only use the static parameter value | |||
| @pytest.mark.xfail | |||
There was a problem hiding this comment.
this should be fixed before we merge this.
There was a problem hiding this comment.
OK - we'll fix the test in future PR
|
@jhamman here is a point simulation for a gridcell that is in Siberia, with ~60% grassland (max snow albedo of 0.7) and ~35% bare soil:
|
| @@ -2,7 +2,8 @@ | |||
| from vic.vic import ffi | |||
| from vic import lib as vic_lib | |||
|
|
|||
|
|
|||
| # TODO: design new test to not only use the static parameter value | |||
| @pytest.mark.xfail | |||
| @@ -970,7 +970,7 @@ check_init_state_file(void) | |||
| dimlen = get_nc_dimension(&(filenames.init_state), "lake_node"); | |||
| if (dimlen != options.Nlakenode) { | |||
| log_err("Number of lake nodes in state file does not " | |||
| "match LAKE_NODES (%d) in global parameter file", | |||
| "match LAKE_NODES (%zu) in global parameter file", | |||
There was a problem hiding this comment.
It's OK for now, but I would have preferred this as a separate PR to keep the individual PRs as focused as possible. Where this would break things is if we had to roll-back a PR for some reason. If we rolled back the MAX_SNOW_ALBEDO PR, that would also roll-back this unrelated bug fix. As said, leave this time.
|
@bartnijssen @jhamman I just took a stab at updating the unit test - not sure if this is the best way or not. |
| def test_snow_albedo_new_snow(): | ||
| assert vic_lib.snow_albedo( | ||
| 0.1, 1., 0.7, -77, 3600., 0, | ||
| ffi.cast('char', False)) == vic_lib.param.SNOW_NEW_SNOW_ALB | ||
| ffi.cast('char', False)) <= vic_lib.param.SNOW_NEW_SNOW_ALB |
There was a problem hiding this comment.
I think you want to write a test that checks that the result of snow_albedo on timestep 0 is max_snow_albedo. You may also consider adding an additional test for when MAX_SNOW_ALBEDO is true or false.
There was a problem hiding this comment.
@bartnijssen and I just discussed this and I'm going to update this test in a future PR just so we can go ahead and merge the PR. I just created an issue for it; see #843 for reference.
There was a problem hiding this comment.
Okay. It sounds like you two have a plan in place. I'll just say that I think the best time/place to write tests is in the PR that changes specific bits of code. Its why we have a checkbox in the PR template for new tests added.
This PR adds in a vegetation-dependent maximum snow albedo for new snow option called
MAX_SNOW_ALBEDO. When set toTruein the global parameter file, maximum snow albedo is read in from the parameters file and used throughout the snow routines for new snow albedo except for bare soil.It also includes an unrelated bug fix to
vic/drivers/shared_image/src/vic_restore.cwhich was necessary for being able to successfully compile the model.