Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Clean up esoil #766

Merged
merged 26 commits into from
Jan 12, 2018
Merged

Clean up esoil #766

merged 26 commits into from
Jan 12, 2018

Conversation

tbohn
Copy link
Contributor

@tbohn tbohn commented Dec 31, 2017 •
edited
Loading

Description (pasted from issue #765):

  • Problem: the logic in computing soil evaporation (esoil) is not very clean nor does it make clear which portion of ET comes from esoil, until the partitioning is cleared up in put_data().
  • Another problem: the default value of RARC_SOIL (architectural resistance for soil evaporation) is currently set to 100 s/m. Evaluation of VIC performance at Ameriflux towers suggests a more optimal value would be 250 s/m.
  • Solution: (a) introduce explicit esoil, transp, and evap terms into the layer struct; (b) remove the bare_evap_frac term; (c) modify logic in func_surf_energy_bal() and other functions to use these terms; (d) make the logic cleaner; and (e) change the default value of RARC_SOIL to 250.0 s/m.
  • Note: I've since rolled back the change to RARC_SOIL, due to this perhaps not being the best way to address the problem (and it being of a different nature than a simple code clean-up).

Note: this PR includes the changes from PR #723. So the diff shows both what's in #723 and what's new to this PR. Probably ought to wait until #723 is resolved before reviewing this one.

tbohn and others added 18 commits July 12, 2017 13:14
@tbohn
Cleanup after merging with develop, which seemed to have removed my c…
f555551 
…hanges.
@tbohn
Simplified prepare_full_energy() to operate on a single instance of c…
acb99ae 
…ell_data and energy_bal structs (removed loop over bands).
@tbohn
Removed re-formatting from uncrustify.
ffad6a7
Merge branch 'develop' into feature/clean_up_vic_run
56231bf
Changed "fcan" to "fcanopy" in vic/drivers/image/src/vic_force.c to b…
8afe2bb 
…e consistent with vic_init.c
Fixed typo.
0d6f3f2
Removing MAX_VEG and MAX_BAND...
294b251
Removed MAX_VEG and MAX_BAND. Replaced arrays that were statically al…
cf4755c 
…located using these terms with dynamically allocated arrays.
A few more fixes.
eded5ae
Backed out changes to validation of bare soil tile.
52f4df6
Changed LAI_IN to LAI.
85a77c0
Changed "lai" to "LAI" in vic_force.c for consistency with vic_init.c.
2d5e8ea
Changed LAI_IN to LAI. Updated docs to use FCANOPY instead of VEGCOVE…
19afd14 
…R. Updated netcdf names of forcing variables from fcan and alb to fcanopy and albedo.
Backed out changes made to bare soil and Cv sum logic.
1c59dbd
Changed LAI_IN to LAI.
957bade
Fixed typos in docs and replaced warning with error in vic_init.c whe…
7a0ea86 
…n snow band areas sum to 0.
Fixed a couple more typos.
f9a9ccb
Cleaned up esoil logic and changed default value of RARC_SOIL to 250.
d2aefec
@jhamman jhamman requested a review from yixinmao January 2, 2018 05:22
jhamman
jhamman reviewed Jan 2, 2018
View reviewed changes
Copy link
Member

@jhamman jhamman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just took a quick look. This looks like a nice set of cleanups. I'm hoping @yixinmao can give this a thorough review.

vic/drivers/shared_all/src/initialize_parameters.c Outdated
@@ -55,7 +55,7 @@ initialize_parameters()
param.EMISS_H2O = 0.98;

// Soil Constraints
param.SOIL_RARC = 100.0;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you explain this change?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've done some more comprehensive evaluations of VIC at the Ameriflux towers and found that 250 s/m is actually a more optimal value than 100.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fine but this change should probably go in a separate PR.

Copy link
Contributor Author

@tbohn tbohn Jan 2, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I've broken out the SOIL_RARC change into a separate issue (#767) and will back out that change from this PR. I'll make a new PR for the SOIL_RARC change.

@jhamman
Copy link
Member

jhamman commented Jan 12, 2018

Okay, I'm going to merge this. I'll do a final uncrustify before 5.1.

@jhamman jhamman merged commit f2b519e into UW-Hydro:develop Jan 12, 2018
jhamman pushed a commit that referenced this pull request Jan 16, 2018
Revert "Clean up esoil (#766)"
e640ff7 
This reverts commit f2b519e.
@jhamman jhamman mentioned this pull request Jan 16, 2018
Merged
@jhamman
Copy link
Member

jhamman commented Jan 16, 2018

This was merged prematurely. I meant to merge #723. This is being reverted in #772.

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

@jhamman jhamman jhamman left review comments

@yixinmao yixinmao Awaiting requested review from yixinmao

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@tbohn @jhamman