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 to state file i/o problems and refactoring of initialization #464

Merged
merged 11 commits into from Apr 14, 2016

Conversation

Projects
None yet
4 participants
@tbohn
Collaborator

tbohn commented Apr 8, 2016

Fix to state file i/o problems and refactoring of initialization into separate functions for generating a default state and for handling derived state variables. Most of these functions are shared across drivers.

closes #457
closes #424
closes #401

Quick summary of the changes:

  1. Fixes for miscellaneous state file i/o issues: e.g., lat/lon indexing was wrong for the 1d case in both vic_store() and vic_restore(); some other variables were being written incorrectly.
  2. Refactoring of state initialization: I made the classic driver look more like the image and cesm drivers, with functions devoted to generating the default state, and handling derived state variables, shared across all drivers.

New structure:

read_soilparam(classic) or vic_init(image/cesm):

  • reads soil parameters, does computation of derived soil parameters and properties
  • moved the setting of soil thermal node spacing to here
  • (image only) calls initialize_* functions

vic_populate_model_state(all drivers):

  • (classic only) calls initialize_* functions
  • if (options.INIT_STATE): reads initial state file, via read_initial_model_state(classic) or vic_restore(image/cesm); moved the capping of initial soil moisture to max_moist to inside read_initial_model_state() (classic only) - I can add similar code to vic_restore() if desired
  • else:generate_default_state(all) and generate_default_lake_state(all) // initializes soil/lake water storage and soil/lake temperatures to default values
  • compute_derived_state_vars(all) and compute_derived_lake_dimensions(all) // sets soil thermal node properties and ice contents, and computes lake dimensions that depend on lake storage

tbohn added some commits Apr 8, 2016

Fix to state file i/o problems and refactoring of initialization into…
… separate functions for generating a default state and for handling derived state variables.
@tbohn

This comment has been minimized.

Show comment
Hide comment
@tbohn

tbohn Apr 8, 2016

Collaborator

Note: I've tested that both classic and image drivers now successfully save state at the appointed time, and either read the specified state file or start from a default state as requested.

Note: results are a little different between the new classic and 4.2. Not hugely, but still, not the same. Hopefully this can be investigated further in restart tests.

Collaborator

tbohn commented Apr 8, 2016

Note: I've tested that both classic and image drivers now successfully save state at the appointed time, and either read the specified state file or start from a default state as requested.

Note: results are a little different between the new classic and 4.2. Not hugely, but still, not the same. Hopefully this can be investigated further in restart tests.

@tbohn

This comment has been minimized.

Show comment
Hide comment
@tbohn

tbohn Apr 9, 2016

Collaborator

Another note: strangely, starting from the the default state does not generate root_brent errors anymore after this PR is merged. However, starting from a state file still does. Not sure why.

Collaborator

tbohn commented Apr 9, 2016

Another note: strangely, starting from the the default state does not generate root_brent errors anymore after this PR is merged. However, starting from a state file still does. Not sure why.

@@ -692,6 +696,108 @@ read_soilparam(FILE *soilparam,
temp.Ws = temp.Ws / temp.max_moist[layer];
}
// Soil thermal node thicknesses and positions

This comment has been minimized.

@tbohn

tbohn Apr 9, 2016

Collaborator

This section was originally in the old initialize_model_state() function, but it really should go here, as node spacing is completely determined from soil parameters and global param options. This way, all soil_con values are populated by the time we exit read_soilparam().

@tbohn

tbohn Apr 9, 2016

Collaborator

This section was originally in the old initialize_model_state() function, but it really should go here, as node spacing is completely determined from soil parameters and global param options. This way, all soil_con values are populated by the time we exit read_soilparam().

This comment has been minimized.

@yixinmao

yixinmao Apr 12, 2016

Contributor

It seems that currently the last section (around the last ~15 lines) in initialize_soil.c also derives some soil_con values. Should that part be moved here to read_soilparam() also?

@yixinmao

yixinmao Apr 12, 2016

Contributor

It seems that currently the last section (around the last ~15 lines) in initialize_soil.c also derives some soil_con values. Should that part be moved here to read_soilparam() also?

This comment has been minimized.

@tbohn

tbohn Apr 13, 2016

Collaborator

Yes, I think you are right. Good catch. I need to move those into both read_soilparam() and vic_init().

@tbohn

tbohn Apr 13, 2016

Collaborator

Yes, I think you are right. Good catch. I need to move those into both read_soilparam() and vic_init().

@@ -792,6 +796,120 @@ vic_init(void)
soil_moisture_from_water_table(&(soil_con[i]), options.Nlayer);
// Soil thermal node thicknesses and positions

This comment has been minimized.

@tbohn

tbohn Apr 9, 2016

Collaborator

This section was originally in the old initialize_model_state() function, but it really should go here, as node spacing is completely determined from soil parameters and global param options. This way, all soil_con values are populated by the time we exit vic_init().

@tbohn

tbohn Apr 9, 2016

Collaborator

This section was originally in the old initialize_model_state() function, but it really should go here, as node spacing is completely determined from soil parameters and global param options. This way, all soil_con values are populated by the time we exit vic_init().

This comment has been minimized.

@jhamman

jhamman Apr 9, 2016

Member

Just out of curiosity. What happens if we write an state file with one set of thermal node properties/dimensions and try to use that initial state to startup a new simulation with a different set of thermal node properties/dimensions? Do we raise an error if the run settings do not match the statefile?

@jhamman

jhamman Apr 9, 2016

Member

Just out of curiosity. What happens if we write an state file with one set of thermal node properties/dimensions and try to use that initial state to startup a new simulation with a different set of thermal node properties/dimensions? Do we raise an error if the run settings do not match the statefile?

This comment has been minimized.

@tbohn

tbohn Apr 9, 2016

Collaborator

There's already a check for that in vic_restore.c, line 1012 - a mismatch raises an error.

@tbohn

tbohn Apr 9, 2016

Collaborator

There's already a check for that in vic_restore.c, line 1012 - a mismatch raises an error.

"STATEDAY and STATESEC are set correctly in your global "
"parameter file.", filenames.statefile,
global_param.stateyear, global_param.statemonth,
global_param.stateday, global_param.statesec);

This comment has been minimized.

@jhamman

jhamman Apr 9, 2016

Member

Same comment as above on seconds vs. hours.

@jhamman

jhamman Apr 9, 2016

Member

Same comment as above on seconds vs. hours.

This comment has been minimized.

@tbohn

tbohn Apr 9, 2016

Collaborator

OK

@tbohn

tbohn Apr 9, 2016

Collaborator

OK

}
// Override possible bad values of soil moisture under lake coming from state file
// (ideally we wouldn't store these in the state file in the first place)

This comment has been minimized.

@jhamman

jhamman Apr 9, 2016

Member

Do we have an open issue for this case (ideally we wouldn't ...).

@jhamman

jhamman Apr 9, 2016

Member

Do we have an open issue for this case (ideally we wouldn't ...).

This comment has been minimized.

@tbohn

tbohn Apr 9, 2016

Collaborator

Yes, #398

@tbohn

tbohn Apr 9, 2016

Collaborator

Yes, #398

if (!assert_close_double(dvar[j],
global_domain.locations[j *
global_domain.n_nx]
.latitude, rtol,

This comment has been minimized.

@jhamman

jhamman Apr 9, 2016

Member

So this just checks the latitudes in the furthest left (west) grid cells? Why not loop over global_domain.ncells and use the io_index to check all coordinate locations?

@jhamman

jhamman Apr 9, 2016

Member

So this just checks the latitudes in the furthest left (west) grid cells? Why not loop over global_domain.ncells and use the io_index to check all coordinate locations?

This comment has been minimized.

@tbohn

tbohn Apr 9, 2016

Collaborator

This is for the 1-d lat/lon case - as determined by the grid domain file, not the state file. Accordingly, if it's the 1-d case, then all lats will match the furthest west ones (in the grid from the grid domain file, parameter file, and forcings, since they are all consistent, right?). Meanwhile, again because its the 1-d case, there is only 1 dimension to check in the state file's lat var - one lat value per grid row. We already checked earlier whether the dimensionality of the lat and lon variables was what it should be. So this check should be sufficient. At least I think so.

@tbohn

tbohn Apr 9, 2016

Collaborator

This is for the 1-d lat/lon case - as determined by the grid domain file, not the state file. Accordingly, if it's the 1-d case, then all lats will match the furthest west ones (in the grid from the grid domain file, parameter file, and forcings, since they are all consistent, right?). Meanwhile, again because its the 1-d case, there is only 1 dimension to check in the state file's lat var - one lat value per grid row. We already checked earlier whether the dimensionality of the lat and lon variables was what it should be. So this check should be sufficient. At least I think so.

This comment has been minimized.

@jhamman

jhamman Apr 10, 2016

Member

sounds good.

@jhamman

jhamman Apr 10, 2016

Member

sounds good.

Show outdated Hide outdated vic/vic_run/src/lakes.eb.c Outdated
@jhamman

This comment has been minimized.

Show comment
Hide comment
@jhamman

jhamman Apr 9, 2016

Member

Nice work @tbohn. Looks good. @yixinmao - you're up!

Member

jhamman commented Apr 9, 2016

Nice work @tbohn. Looks good. @yixinmao - you're up!

{
extern option_struct options;
extern option_struct options;

This comment has been minimized.

@yixinmao

yixinmao Apr 11, 2016

Contributor

Smaller space?

@yixinmao

yixinmao Apr 11, 2016

Contributor

Smaller space?

This comment has been minimized.

@bartnijssen

bartnijssen Apr 11, 2016

Member

uncrustify should take care of this. Make sure the code is uncrustified before it gets merged.

@bartnijssen

bartnijssen Apr 11, 2016

Member

uncrustify should take care of this. Make sure the code is uncrustified before it gets merged.

This comment has been minimized.

@tbohn

tbohn Apr 11, 2016

Collaborator

But I did run uncrustify... that change happened because of uncrustify...

@tbohn

tbohn Apr 11, 2016

Collaborator

But I did run uncrustify... that change happened because of uncrustify...

This comment has been minimized.

@jhamman

jhamman Apr 11, 2016

Member

This is aligned with the section of variables below. No need to make further edits.

@jhamman

jhamman Apr 11, 2016

Member

This is aligned with the section of variables below. No need to make further edits.

false);
}
initialize_energy(energy, Nveg);

This comment has been minimized.

@yixinmao

yixinmao Apr 11, 2016

Contributor

Currently, in the following, if (options.INIT_STATE) reads in initial state file and modifies some direct states (i.e., states that cannot be derived and must be saved), supposedly; else if no initial state, then a part of states are modified. Then compute_derived_state_vars again modifies a part of states that are derived. Then the rest of the states might be derived and modified in the vic_run core code.

To me this seems a bit confusing because it's not very clear which state variables are 'direct' and must be saved and read from initial state file, and which are 'derived'; and it is possible that some 'direct' states are not correctly saved and read, causing inexact restart (as I'm seeing now for a simple bare soil case). Would it be possible to separate 'direct' and 'derived' states in a cleaner way? For example, maybe not call initialize_* functions at all, and then in if (options.INIT_STATE), read in all 'direct' states from state file (as it is doing now supposedly); in else, set the same set of 'direct' states to their default values (I guess currently many of the default values are zero, so they are assigned above in initialize_*). In this way, all the 'derived' variables will either be assigned in compute_derived_state_vars, or in the core vic_run computation; but if any 'derived' variables are used before being assigned, then there would be an error and we should avoid it from happening.

These are just some thoughts based on my understanding of the code structure, and not sure if makes sense at all...

@yixinmao

yixinmao Apr 11, 2016

Contributor

Currently, in the following, if (options.INIT_STATE) reads in initial state file and modifies some direct states (i.e., states that cannot be derived and must be saved), supposedly; else if no initial state, then a part of states are modified. Then compute_derived_state_vars again modifies a part of states that are derived. Then the rest of the states might be derived and modified in the vic_run core code.

To me this seems a bit confusing because it's not very clear which state variables are 'direct' and must be saved and read from initial state file, and which are 'derived'; and it is possible that some 'direct' states are not correctly saved and read, causing inexact restart (as I'm seeing now for a simple bare soil case). Would it be possible to separate 'direct' and 'derived' states in a cleaner way? For example, maybe not call initialize_* functions at all, and then in if (options.INIT_STATE), read in all 'direct' states from state file (as it is doing now supposedly); in else, set the same set of 'direct' states to their default values (I guess currently many of the default values are zero, so they are assigned above in initialize_*). In this way, all the 'derived' variables will either be assigned in compute_derived_state_vars, or in the core vic_run computation; but if any 'derived' variables are used before being assigned, then there would be an error and we should avoid it from happening.

These are just some thoughts based on my understanding of the code structure, and not sure if makes sense at all...

This comment has been minimized.

@tbohn

tbohn Apr 11, 2016

Collaborator

@yixinmao - I can't say what the cause of the inexact restarts may be. But to my knowledge, the derived variables are not saved in the state file. All state variables are initialized to 0 in the initialize_* functions (e.g., initialize_soil()). Then, some are overwritten by either reading the state file or generating the default state, and others are subsequently overwritten by create_derived* (this is the same set of variables regardless of whether a state file was read or a default state was set). So if there are any uninitialized variables, they simply need to be added to the initialize* functions.

Yes, the set of variables that are assigned values in generate_default* is smaller than the set of variables stored in the state files. This is because only those variables that need to have non-zero values are set in generate_default*.

There is one main exception to this: there are many lake state variables saved in the state file that are actually redundant, i.e. can be derived from the others. There already is an open issue about this but I think it is slated for 5.1 as it is not essential to getting 5.0 running correctly.

@tbohn

tbohn Apr 11, 2016

Collaborator

@yixinmao - I can't say what the cause of the inexact restarts may be. But to my knowledge, the derived variables are not saved in the state file. All state variables are initialized to 0 in the initialize_* functions (e.g., initialize_soil()). Then, some are overwritten by either reading the state file or generating the default state, and others are subsequently overwritten by create_derived* (this is the same set of variables regardless of whether a state file was read or a default state was set). So if there are any uninitialized variables, they simply need to be added to the initialize* functions.

Yes, the set of variables that are assigned values in generate_default* is smaller than the set of variables stored in the state files. This is because only those variables that need to have non-zero values are set in generate_default*.

There is one main exception to this: there are many lake state variables saved in the state file that are actually redundant, i.e. can be derived from the others. There already is an open issue about this but I think it is slated for 5.1 as it is not essential to getting 5.0 running correctly.

This comment has been minimized.

@bartnijssen

bartnijssen Apr 11, 2016

Member

Isn't that basically the difference between prognostic and diagnostic variables? Ideally we should make sure to only store prognostic variables and recalculate the diagnostic variables, so that they are consistent with the prognostic ones. Please correct me if I am wrong.

@bartnijssen

bartnijssen Apr 11, 2016

Member

Isn't that basically the difference between prognostic and diagnostic variables? Ideally we should make sure to only store prognostic variables and recalculate the diagnostic variables, so that they are consistent with the prognostic ones. Please correct me if I am wrong.

This comment has been minimized.

@tbohn

tbohn Apr 11, 2016

Collaborator

I think you're correct. I think the code is doing essentially what you're describing (with the exception of the lake state variables, which there's an open issue for), although we're not using the same words.

@tbohn

tbohn Apr 11, 2016

Collaborator

I think you're correct. I think the code is doing essentially what you're describing (with the exception of the lake state variables, which there's an open issue for), although we're not using the same words.

This comment has been minimized.

@tbohn

tbohn Apr 11, 2016

Collaborator

So, do we want to have this PR wait until @yixinmao 's restart tests are done (and any necessary bug fixes indicated by them are incorporated into this) or can this PR be merged now and fixes that arise from restart tests can be handled as separate issues? Those fixes might very well lead to better organization of the variables and code into prognostic and diagnostic variables...

@tbohn

tbohn Apr 11, 2016

Collaborator

So, do we want to have this PR wait until @yixinmao 's restart tests are done (and any necessary bug fixes indicated by them are incorporated into this) or can this PR be merged now and fixes that arise from restart tests can be handled as separate issues? Those fixes might very well lead to better organization of the variables and code into prognostic and diagnostic variables...

This comment has been minimized.

@jhamman

jhamman Apr 11, 2016

Member

I do not think we want to wait on getting exact restarts to merge this. That process could take some time and we don't want to hold up other work.

@jhamman

jhamman Apr 11, 2016

Member

I do not think we want to wait on getting exact restarts to merge this. That process could take some time and we don't want to hold up other work.

@bartnijssen

This comment has been minimized.

Show comment
Hide comment
@bartnijssen

bartnijssen Apr 11, 2016

Member

@tbohn : merge conflicts (probably because of Joe's PR that was just merged) - please merge develop into this PR

Member

bartnijssen commented Apr 11, 2016

@tbohn : merge conflicts (probably because of Joe's PR that was just merged) - please merge develop into this PR

tbohn added some commits Apr 11, 2016

Merge branch 'develop' into feature/state_refactor
Conflicts:
	vic/drivers/shared_image/include/vic_driver_shared_image.h
	vic/drivers/shared_image/src/vic_store.c

@jhamman jhamman added this to the 5.0 milestone Apr 11, 2016

@jhamman jhamman added the refactor label Apr 11, 2016

@jhamman jhamman merged commit 407e6ba into UW-Hydro:develop Apr 14, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tbohn tbohn deleted the tbohn:feature/state_refactor branch May 18, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment