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

Clean up and initial fix for exact restart - classic driver #481

Merged
merged 27 commits into from May 2, 2016

Conversation

Projects
None yet
3 participants
@yixinmao
Contributor

yixinmao commented Apr 24, 2016

This PR is addressing part of issue #479 .

Changes related to exact restart:

  • Raised ascii state variable outputs to %.16g
  • Added the prognostic state var energy.Tfoliage to write and read in the state file

Clean up:

  • Cleanly separated state and flux variables in vic_def.h and print_library_shared.c

  • In initialize_*.c, all state and flux variables are now initialized to zero (before some were missing); in initalize_soil.c, moved computation for derived variables into compute_derived_state_vars.

  • In compute_derived_state_vars, only computed the four variables when FROZEN_SOIL=TRUE:

    energy.Cs_node
    energy.kappa_node
    energy.moist
    energy.ice

ymao added some commits Apr 6, 2016

ymao
Separate state and fluxes in vic_def.h and print_library_shared.c
    - Haven't done for lake
    - Separate carbon variables for veg_var
    - All floats are printed with 6 digits after decimal points (instead
      of 4)
ymao
Output higher precision (.16g) ascii state files
    - This change helps reduce (but not eliminate) restarting error when
      using ascii format in classic driver caused by precision error
when writing state file
ymao
When initializing states, only derive the following state variables if
FULL_ENERGY=TRUE or FROZEN_SOIL=TRUE; otherwise, these variables will
not be used, so don't need to be initialized:
    energy.Cs_node
    energy.kappa_node
    energy.moist
    energy.ice
ymao
Merge remote-tracking branch 'remotes/upstream/develop' into restart/…
…classic_driver

Conflicts:
	vic/vic_run/include/vic_def.h
Show outdated Hide outdated vic/drivers/image/Makefile Outdated
Show outdated Hide outdated vic/vic_run/src/surface_fluxes.c Outdated
@jhamman

This comment has been minimized.

Show comment
Hide comment
@jhamman

jhamman Apr 25, 2016

Member

@yixinmao - I'm made a few minor comments. You have failing tests on all drivers and there are a bunch of compiler warnings to address. Once you've cleaned thigs up here, ping @tbohn for the next review.

Member

jhamman commented Apr 25, 2016

@yixinmao - I'm made a few minor comments. You have failing tests on all drivers and there are a bunch of compiler warnings to address. Once you've cleaned thigs up here, ping @tbohn for the next review.

@yixinmao

This comment has been minimized.

Show comment
Hide comment
@yixinmao

yixinmao Apr 25, 2016

Contributor

@jhamman I have addressed all your comments (thank you!). @tbohn do you want to take a look at the current version again?

Contributor

yixinmao commented Apr 25, 2016

@jhamman I have addressed all your comments (thank you!). @tbohn do you want to take a look at the current version again?

}
}
/* Write foliage temperature */

This comment has been minimized.

@tbohn

tbohn Apr 27, 2016

Collaborator

Need to also increment the Nbytes count to include the size of Tfoliage, on line 103 (I can't seem to add an inline comment to that line of this diff, so I'm adding the note here). You can look at how the other variables are done and pattern Tfoliage after them. It should be a simple 1-liner.

@tbohn

tbohn Apr 27, 2016

Collaborator

Need to also increment the Nbytes count to include the size of Tfoliage, on line 103 (I can't seem to add an inline comment to that line of this diff, so I'm adding the note here). You can look at how the other variables are done and pattern Tfoliage after them. It should be a simple 1-liner.

This comment has been minimized.

@yixinmao

yixinmao Apr 27, 2016

Contributor

Good point! Will do. I haven't really tested for binary format - should we actually have a binary version of the sample data?

@yixinmao

yixinmao Apr 27, 2016

Contributor

Good point! Will do. I haven't really tested for binary format - should we actually have a binary version of the sample data?

This comment has been minimized.

@tbohn

tbohn Apr 27, 2016

Collaborator

Do you mean a binary version of the initial state file? (whether the state files are binary is independent of whether the forcings or outputs are binary)

@tbohn

tbohn Apr 27, 2016

Collaborator

Do you mean a binary version of the initial state file? (whether the state files are binary is independent of whether the forcings or outputs are binary)

This comment has been minimized.

@yixinmao

yixinmao Apr 27, 2016

Contributor

I see. I was thinking about a complete binary sample dataset, but specifically to this problem testing binary i/o for the state file is enough. Thanks!

@yixinmao

yixinmao Apr 27, 2016

Contributor

I see. I was thinking about a complete binary sample dataset, but specifically to this problem testing binary i/o for the state file is enough. Thanks!

veg_var[i][j].albedo = 0.0;
veg_var[i][j].displacement = 0.0;
veg_var[i][j].fcanopy = 0.0;
veg_var[i][j].LAI = 0.0;
veg_var[i][j].roughness = 0.0;
veg_var[i][j].throughfall = 0.0;
veg_var[i][j].Wdew = 0.0;
veg_var[i][j].Wdmax = 0.0;

This comment has been minimized.

@tbohn

tbohn Apr 27, 2016

Collaborator

Technically, Wdmax is a derived (or "diagnostic", I suppose) state, completely derived from LAI. However, both LAI and Wdmax get updated at similar times (every time that LAI gets a new value, Wdmax is recomputed), so keeping the initialization of Wdmax here would be completely consistent with that other behavior. It's your (and/or Bart and Joe's) call.

@tbohn

tbohn Apr 27, 2016

Collaborator

Technically, Wdmax is a derived (or "diagnostic", I suppose) state, completely derived from LAI. However, both LAI and Wdmax get updated at similar times (every time that LAI gets a new value, Wdmax is recomputed), so keeping the initialization of Wdmax here would be completely consistent with that other behavior. It's your (and/or Bart and Joe's) call.

This comment has been minimized.

@yixinmao

yixinmao Apr 27, 2016

Contributor

Hmm what exactly do you mean and what do you suggest we should do?

@yixinmao

yixinmao Apr 27, 2016

Contributor

Hmm what exactly do you mean and what do you suggest we should do?

This comment has been minimized.

@tbohn

tbohn Apr 27, 2016

Collaborator

Personally I'm OK with leaving it as it currently is. I was just pointing out that Wdmax is derived from LAI - it's good to know this if we want to avoid future problems similar to the one you just fixed (for example, someone who doesn't know it's derived from LAI might change LAI somewhere else in the code and forget to include an update to Wdmax). I don't know if there is a different way to handle updating Wdmax that is not also cumbersome (since LAI is updated in a few different places). I suppose one way to handle it could be to eliminate Wdmax from veg_var, veg_con, and veg_lib structures, and instead rewrite the logic that uses Wdmax to compute Wdmax from LAI on the spot immediately before it is used, and for that particular Wdmax to be a temporary variable that exists only within that function. But that might be too much trouble. @jhamman and @bartnijssen might want to weight in on this.

@tbohn

tbohn Apr 27, 2016

Collaborator

Personally I'm OK with leaving it as it currently is. I was just pointing out that Wdmax is derived from LAI - it's good to know this if we want to avoid future problems similar to the one you just fixed (for example, someone who doesn't know it's derived from LAI might change LAI somewhere else in the code and forget to include an update to Wdmax). I don't know if there is a different way to handle updating Wdmax that is not also cumbersome (since LAI is updated in a few different places). I suppose one way to handle it could be to eliminate Wdmax from veg_var, veg_con, and veg_lib structures, and instead rewrite the logic that uses Wdmax to compute Wdmax from LAI on the spot immediately before it is used, and for that particular Wdmax to be a temporary variable that exists only within that function. But that might be too much trouble. @jhamman and @bartnijssen might want to weight in on this.

This comment has been minimized.

@jhamman

jhamman Apr 29, 2016

Member

I agree that Wdmax isn't really a state or flux variable, it is really a derived model parameter. my suggestion would be to open a new issue that rethinks the handling of Wdmax and we can tackle that in another PR sometime down the road.

@jhamman

jhamman Apr 29, 2016

Member

I agree that Wdmax isn't really a state or flux variable, it is really a derived model parameter. my suggestion would be to open a new issue that rethinks the handling of Wdmax and we can tackle that in another PR sometime down the road.

This comment has been minimized.

@jhamman

jhamman May 2, 2016

Member

xref #488

@jhamman

jhamman May 2, 2016

Member

xref #488

@tbohn

This comment has been minimized.

Show comment
Hide comment
@tbohn

tbohn Apr 27, 2016

Collaborator

@yixinmao I've finished looking it over. Other than my 2 comments, it looks good. This is of course an important improvement in making the restarts more accurate. It also helps clean up the code, which will help us maintain the code in the future.

Collaborator

tbohn commented Apr 27, 2016

@yixinmao I've finished looking it over. Other than my 2 comments, it looks good. This is of course an important improvement in making the restarts more accurate. It also helps clean up the code, which will help us maintain the code in the future.

@jhamman jhamman merged commit 1a9888b into UW-Hydro:develop May 2, 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