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

Fixed exact restart when FROZEN_SOIL=TRUE #509

Merged
merged 29 commits into from May 27, 2016

Conversation

Projects
None yet
3 participants
@yixinmao
Contributor

yixinmao commented May 24, 2016

This PR fixed the inexact restart when FROZEN_SOIL=TRUE. Specifically:

  • Before, the layer ice content and layer temperatures are calculated together in a single function (estimate_layer_ice_content or estimate_layer_ice_content_quick_flux). Now, the calculation for layer ice content and layer temperatures are done in separated functions
  • At the beginning of model run, the calculation for layer ice content is moved from compute_derived_state_vars to generate_default_state, because layer ice content only needs to be initialized when there is no initial state file; when there is an initial state file, layer ice content should be read from the file. The calculation for layer temperature remains in compute_derived_state_vars, since it is a prognostic state variable.

ymao added some commits May 16, 2016

ymao
Save/read energy.LongUnderOut and energ.snow_flux to the state file -
for both classic and image driver
(This is a temporary solution to ensure exact restart)
ymao
Fixed exact restart when FROZEN_SOIL=TRUE and FULL_ENERGY=TRUE:
    - Finished separating `estimate_layer_ice_content`
    - Moved calculation of `cell.layer.ice` from
      `compute_derived_state_vars` to `generate_default_state`
ymao
Changed the order of calculating layer ice content and temperature -
for clarity purpose; does not affect results
ymao
Moved calculation for layer ice content from `compute_derived_sate_vars`
to `generate_default_state` when QUICK_FLUX=TRUE - this resolves the
exact restart issue when FROZEN_SOIL=TRUE and FULL_ENERGY=TRUE for
QUICK_FLUX=TRUE
@jhamman

This comment has been minimized.

Show comment
Hide comment
@jhamman

jhamman May 24, 2016

Member

A few build warnings that should be addressed:

../../vic_run/src/soil_conduction.c: In function ‘estimate_frost_temperature_and_depth’:

../../vic_run/src/soil_conduction.c:324:57: warning: unused parameter ‘layer’ [-Wunused-parameter]

../../vic_run/src/soil_conduction.c:426:1: warning: control reaches end of non-void function [-Wreturn-type]

../shared_all/src/compute_derived_state_vars.c: In function ‘compute_derived_state_vars’:

../shared_all/src/compute_derived_state_vars.c:53:32: warning: variable ‘ice’ set but not used [-Wunused-but-set-variable]
Member

jhamman commented May 24, 2016

A few build warnings that should be addressed:

../../vic_run/src/soil_conduction.c: In function ‘estimate_frost_temperature_and_depth’:

../../vic_run/src/soil_conduction.c:324:57: warning: unused parameter ‘layer’ [-Wunused-parameter]

../../vic_run/src/soil_conduction.c:426:1: warning: control reaches end of non-void function [-Wreturn-type]

../shared_all/src/compute_derived_state_vars.c: In function ‘compute_derived_state_vars’:

../shared_all/src/compute_derived_state_vars.c:53:32: warning: variable ‘ice’ set but not used [-Wunused-but-set-variable]
@yixinmao

This comment has been minimized.

Show comment
Hide comment
@yixinmao

yixinmao May 24, 2016

Contributor

@jhamman Yes thanks! I'll update those.

Contributor

yixinmao commented May 24, 2016

@jhamman Yes thanks! I'll update those.

@yixinmao

This comment has been minimized.

Show comment
Hide comment
@yixinmao

yixinmao May 24, 2016

Contributor

@jhamman I've cleaned up some unused variables.

Contributor

yixinmao commented May 24, 2016

@jhamman I've cleaned up some unused variables.

@bartnijssen

This comment has been minimized.

Show comment
Hide comment
@bartnijssen

bartnijssen May 24, 2016

Member

@yixinmao : Please be specific on what was fixed and what still remains for fixing? When you say "some" of the variable that is unclear.

Member

bartnijssen commented May 24, 2016

@yixinmao : Please be specific on what was fixed and what still remains for fixing? When you say "some" of the variable that is unclear.

if (ErrorFlag == ERROR) {
log_err("Error in "
"estimate_layer_ice_content_quick_flux");
"estimate_layer_temperature_quick_flux");

This comment has been minimized.

@bartnijssen

bartnijssen May 24, 2016

Member

Change to:

    log_err("Error in estimate_layer_temperature_quick_flux");

unless this is an uncrustify result in which case just leave it.

@bartnijssen

bartnijssen May 24, 2016

Member

Change to:

    log_err("Error in estimate_layer_temperature_quick_flux");

unless this is an uncrustify result in which case just leave it.

This comment has been minimized.

@jhamman

jhamman May 27, 2016

Member

@yixinmao, what's going on here?

@jhamman

jhamman May 27, 2016

Member

@yixinmao, what's going on here?

This comment has been minimized.

@jhamman

jhamman May 27, 2016

Member

oh I see. Go ahead and leave as is.

@jhamman

jhamman May 27, 2016

Member

oh I see. Go ahead and leave as is.

Show outdated Hide outdated vic/vic_run/include/vic_run.h Outdated
@yixinmao

This comment has been minimized.

Show comment
Hide comment
@yixinmao

yixinmao May 24, 2016

Contributor

@bartnijssen @jhamman : To clarify: I've cleaned up the warnings in response to Joe's comment.

Contributor

yixinmao commented May 24, 2016

@bartnijssen @jhamman : To clarify: I've cleaned up the warnings in response to Joe's comment.

@jhamman

This comment has been minimized.

Show comment
Hide comment
@jhamman

jhamman May 26, 2016

Member

@yixinmao - ping me for review after you've merged yixinmao#1.

Member

jhamman commented May 26, 2016

@yixinmao - ping me for review after you've merged yixinmao#1.

bartnijssen and others added some commits May 26, 2016

Merge pull request #1 from bartnijssen/yixinmao-restart/image_driver_…
…EB_FS

remove hard-coded, statically allocated array sizes for tmpT and tmpZ
ymao
Merge remote-tracking branch 'remotes/origin/restart/image_driver_EB_…
…FS' into restart/image_driver_EB_FS

Conflicts:
	vic/vic_run/include/vic_run.h
@yixinmao

This comment has been minimized.

Show comment
Hide comment
@yixinmao

yixinmao May 27, 2016

Contributor

@jhamman: I've addressed issue #511 and is ready to be merged (once the travis test is passed), unless you have further comments.

Contributor

yixinmao commented May 27, 2016

@jhamman: I've addressed issue #511 and is ready to be merged (once the travis test is passed), unless you have further comments.

@jhamman

This comment has been minimized.

Show comment
Hide comment
@jhamman

jhamman May 27, 2016

Member

@yixinmao - we just need an additional comment to the release notes. Just explain this change in the existing exact restarts bug fix section and reference the relevant github issues.

Member

jhamman commented May 27, 2016

@yixinmao - we just need an additional comment to the release notes. Just explain this change in the existing exact restarts bug fix section and reference the relevant github issues.

@yixinmao

This comment has been minimized.

Show comment
Hide comment
@yixinmao

yixinmao May 27, 2016

Contributor

@jhamman : release notes updated, ready to merge once travis build passed. Also, issue #511 can be closed. Thanks!

Contributor

yixinmao commented May 27, 2016

@jhamman : release notes updated, ready to merge once travis build passed. Also, issue #511 can be closed. Thanks!

@jhamman jhamman merged commit 5794c54 into UW-Hydro:develop May 27, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
size_t frost_area;
size_t tmpTshape[] = {
options.Nlayer, options.Nnode,
options.Nfrost + 1

This comment has been minimized.

@bartnijssen

bartnijssen May 28, 2016

Member

Cool - I did not know that that actually worked!

@bartnijssen

bartnijssen May 28, 2016

Member

Cool - I did not know that that actually worked!

@yixinmao yixinmao deleted the yixinmao:restart/image_driver_EB_FS branch Jun 1, 2016

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