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

Fix dz_node and node_depth in image statefile #657

Merged
merged 8 commits into from Dec 23, 2016

Conversation

Projects
None yet
4 participants
@yixinmao
Contributor

yixinmao commented Dec 8, 2016

In image driver, dz_node and node_depth was assumed to be not spatially-distributed, which was wrong. Now output spatially distributed dz_node and node_depth in the output state file; also, when reading in an initial state file, check these variables at all grid cells to see if they match the values calculated from soil parameters.

@bartnijssen

This comment has been minimized.

Show comment
Hide comment
@bartnijssen

bartnijssen Dec 9, 2016

Member

Do we need to store them or do we just need to make sure that they are calculated every time the model starts?

Member

bartnijssen commented Dec 9, 2016

Do we need to store them or do we just need to make sure that they are calculated every time the model starts?

@yixinmao

This comment has been minimized.

Show comment
Hide comment
@yixinmao

yixinmao Dec 9, 2016

Contributor

@bartnijssen Currently they are already calculated every time the model starts, and then in image driver it further checks if the calculated values match those in the initial state file (classic driver does not do the check; it always use those saved in the initial state file). I guess for model running purpose, we don't have to store them; for practical purpose (user wants to know node depth info), it might be informative - or we could write it out to history file if user outputs node-related variables.

Contributor

yixinmao commented Dec 9, 2016

@bartnijssen Currently they are already calculated every time the model starts, and then in image driver it further checks if the calculated values match those in the initial state file (classic driver does not do the check; it always use those saved in the initial state file). I guess for model running purpose, we don't have to store them; for practical purpose (user wants to know node depth info), it might be informative - or we could write it out to history file if user outputs node-related variables.

@jhamman

This comment has been minimized.

Show comment
Hide comment
@jhamman

jhamman Dec 9, 2016

Member

@bartnijssen and @yixinmao - here's my perspective, we want to write/read coordinate variables like dz_mode for two reasons:

  1. It is useful to be able to inspect a statefile as a standalone object. Since dz_node is not a model parameter or a variable that will exist in the domain file, user would have to reproduce the behavior in the VIC if they wanted to compare a state variable to its VIC calculated coordinate.
  2. Many state variables are not transferable when model options are changed. VIC can calculate dz_node in two ways (linear or exponential), both using the same number of nodes. If we were to stop writing out the node depths, and a user were to change the way the nodes are distributed via a global parameter option, the incorrect state would be applied to their model. In fact, this is the primary use case right now. VIC calculates its node depths at startup and then compares them to those in the statefile, exiting if they differ.
Member

jhamman commented Dec 9, 2016

@bartnijssen and @yixinmao - here's my perspective, we want to write/read coordinate variables like dz_mode for two reasons:

  1. It is useful to be able to inspect a statefile as a standalone object. Since dz_node is not a model parameter or a variable that will exist in the domain file, user would have to reproduce the behavior in the VIC if they wanted to compare a state variable to its VIC calculated coordinate.
  2. Many state variables are not transferable when model options are changed. VIC can calculate dz_node in two ways (linear or exponential), both using the same number of nodes. If we were to stop writing out the node depths, and a user were to change the way the nodes are distributed via a global parameter option, the incorrect state would be applied to their model. In fact, this is the primary use case right now. VIC calculates its node depths at startup and then compares them to those in the statefile, exiting if they differ.
@yixinmao

This comment has been minimized.

Show comment
Hide comment
@yixinmao

yixinmao Dec 9, 2016

Contributor

@jhamman So after the fix in this PR, image driver is doing what you proposed. Should we add the same check for classic driver as well?

Contributor

yixinmao commented Dec 9, 2016

@jhamman So after the fix in this PR, image driver is doing what you proposed. Should we add the same check for classic driver as well?

@jhamman

This comment has been minimized.

Show comment
Hide comment
@jhamman

jhamman Dec 9, 2016

Member

@yixinmao - no. The benefit of using netCDF to store the image driver state is there is a clear and concise way to keep metadata with the state variables. The classic driver doesn't really fit that bill so I would not suggest extending this beyond the image driver.

Member

jhamman commented Dec 9, 2016

@yixinmao - no. The benefit of using netCDF to store the image driver state is there is a clear and concise way to keep metadata with the state variables. The classic driver doesn't really fit that bill so I would not suggest extending this beyond the image driver.

@yixinmao

This comment has been minimized.

Show comment
Hide comment
@yixinmao

yixinmao Dec 9, 2016

Contributor

@jhamman OK - the same two variables (dz_node and node_depth) are already (and have always been) stored in state file in classic driver; and they are recalculated at the beginning of VIC run as well - but just not checked (values in the state file are directly used).

Contributor

yixinmao commented Dec 9, 2016

@jhamman OK - the same two variables (dz_node and node_depth) are already (and have always been) stored in state file in classic driver; and they are recalculated at the beginning of VIC run as well - but just not checked (values in the state file are directly used).

@bartnijssen

This comment has been minimized.

Show comment
Hide comment
@bartnijssen

bartnijssen Dec 23, 2016

Member

@yixinmao : Is this one ready to merge or are we waiting for something (other than me)

Member

bartnijssen commented Dec 23, 2016

@yixinmao : Is this one ready to merge or are we waiting for something (other than me)

@yixinmao

This comment has been minimized.

Show comment
Hide comment
@yixinmao

yixinmao Dec 23, 2016

Contributor

@bartnijssen this should be ready to merge once someone takes a quick review. @jhamman @dgergel could you take a brief look? Most changes shown are due to indenting.

Contributor

yixinmao commented Dec 23, 2016

@bartnijssen this should be ready to merge once someone takes a quick review. @jhamman @dgergel could you take a brief look? Most changes shown are due to indenting.

@jhamman

This comment has been minimized.

Show comment
Hide comment
@jhamman

jhamman Dec 23, 2016

Member

LGTM. After @dgergel takes a look, I think this can go in.

(please "squash and merge" though)

Member

jhamman commented Dec 23, 2016

LGTM. After @dgergel takes a look, I think this can go in.

(please "squash and merge" though)

@dgergel

This comment has been minimized.

Show comment
Hide comment
@dgergel

dgergel Dec 23, 2016

Contributor

@yixinmao and @jhamman this looks good to me. @bartnijssen, ready to merge.

Contributor

dgergel commented Dec 23, 2016

@yixinmao and @jhamman this looks good to me. @bartnijssen, ready to merge.

@bartnijssen bartnijssen merged commit 62f63c8 into UW-Hydro:hotfix/5.0.1 Dec 23, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment