Miscellaneous clean-up and minor fixes. #723

tbohn · 2017-07-11T22:50:02Z

closes Add support for veg_hist forcings (non-climatological) #404, Possibly unnecessary logic in nveg and nv_active relationship in drivers/shared_image #705, Cleanup: miscellaneous cleanup of vic_run #722, Cleanup: How to optimize static array limits MAX_VEG, MAX_BAND, etc #759, Cleanup: Change forcing variable LAI_IN to LAI #763, Cleanup: Change netcdf names for vegetation forcing variables fcanopy and albedo #764
tests passed
new tests added
science test figures
ran uncrustify prior to final commit
ReleaseNotes entry

jhamman · 2017-07-12T19:04:36Z

Thanks @tbohn. Can you update your branch with the updates I merged in to develop today? I'm going to let @dgergel have the first review here and @bartnijssen and I will follow her up.

tbohn · 2017-07-12T19:14:10Z

Actually, @jhamman @dgergel and @bartnijssen can you hold on a little bit? The pull request isn't final yet; I may make another simplification. I will definitely merge the latest develop mods into my branch too.

I'll let you know when this is ready. Thanks!

dgergel · 2017-07-12T19:19:01Z

@tbohn sure. Just let me know when it's ready.

…hanges.

…ell_data and energy_bal structs (removed loop over bands).

tbohn · 2017-07-12T21:15:27Z

vic/vic_run/include/vic_run.h

 double penman(double, double, double, double, double, double, double);
 void photosynth(char, double, double, double, double, double, double, double,
                double, double, char *, double *, double *, double *, double *,
                double *);
 void polint(double xa[], double ya[], int n, double x, double *y, double *dy);
-void prepare_full_energy(int, all_vars_struct *, soil_con_struct *, double *,
+void prepare_full_energy(cell_data_struct *, energy_bal_struct *,
+                         soil_con_struct *, double *,
                         double *);


The change to prepare_full_energy() is the only non-format modification. The rest of the changes in vic_run.h came from running uncrustify.

…e consistent with vic_init.c

…located using these terms with dynamically allocated arrays.

…R. Updated netcdf names of forcing variables from fcan and alb to fcanopy and albedo.

jhamman

I gave this a brief pass and generally like what is going on here. It would be great if @yixinmao and/or @dgergel could give it a full review before we merge it.

jhamman · 2017-12-19T21:06:58Z

docs/Documentation/Drivers/Classic/GlobalParam.md

@@ -218,7 +218,7 @@ The following options describe the input parameter files.
 | VEGPARAM_ALB       | string          | TRUE or FALSE       | If TRUE the vegetation parameter file contains an extra line for each vegetation type that defines monthly ALBEDO values for each vegetation type for each grid cell. Default = FALSE.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                    |
 | ALB_SRC         | string          | N/A                 | This option tells VIC where to look for ALBEDO values:FROM_VEGLIB = Use the ALBEDO values listed in the vegetation library file. FROM_VEGPARAM = Use the ALBEDO values listed in the vegetation parameter file. Note: for this to work, VEGPARAM_ALB must be TRUE.FROM_VEGHIST = Use the ALBEDO values listed in the veg_hist forcing files. Note: for this to work, ALBEDO must be supplied in the veg_hist files and listed in the global parameter file as one of the variables in the files. Default = FROM_VEGLIB.  |
 | VEGPARAM_LAI       | string          | TRUE or FALSE       | If TRUE the vegetation parameter file contains an extra line for each vegetation type that defines monthly LAI values for each vegetation type for each grid cell. Default = FALSE.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                       |
-| LAI_SRC            | string          | N/A                 | This option tells VIC where to look for LAI values:FROM_VEGLIB = Use the LAI values listed in the vegetation library file.FROM_VEGPARAM = Use the LAI values listed in the vegetation parameter file. Note: for this to work, VEGPARAM_LAI must be TRUE.FROM_VEGHIST = Use the LAI values listed in the veg_hist forcing files. Note: for this to work, LAI_IN must be supplied in the veg_hist files and listd in the global parameter file as one of the variables in the files. Default = FROM_VEGLIB.                                                                                                                                                                 |
+| LAI_SRC            | string          | N/A                 | This option tells VIC where to look for LAI values:FROM_VEGLIB = Use the LAI values listed in the vegetation library file.FROM_VEGPARAM = Use the LAI values listed in the vegetation parameter file. Note: for this to work, VEGPARAM_LAI must be TRUE.FROM_VEGHIST = Use the LAI values listed in the veg_hist forcing files. Note: for this to work, LAI must be supplied in the veg_hist files and listd in the global parameter file as one of the variables in the files. Default = FROM_VEGLIB.                                                                                                                                                                 |


it looks like these typos were already present but can you add a space after values: and after TRUE.?

jhamman · 2017-12-19T21:07:50Z

docs/Documentation/Drivers/Image/GlobalParam.md

@@ -176,7 +176,7 @@ The following options describe the input parameter files.
 | JULY_TAVG_SUPPLIED | string | TRUE or FALSE | If TRUE then VIC will expect an additional variable in the parameter file (July_Tavg) to contain the grid cell's average July temperature. *NOTE*: Supplying July average temperature is only required if the COMPUTE_TREELINE option is set to TRUE. <br><br>Default = FALSE.                                                                                                                                                                                                                                                                                                                                                                                            |
 | ORGANIC_FRACT      | string | TRUE or FALSE | TRUE = the parameter file contains extra variables: the organic fraction, and the bulk density and soil particle density of the organic matter in each soil layer. FALSE = the parameter file does not contain any information about organic soil, and organic fraction should be assumed to be 0. <br><br>Default = FALSE.                                                                                                                                                                                                                                                                                                                                               |
 | ALB_SRC         | string          | N/A                 | This option tells VIC where to look for ALBEDO values: if FROM_VEGLIB or FROM_VEGPARAM = Use the ALBEDO values from the parameter file.FROM_VEGPARAM = Use the ALBEDO values listed in the vegetation parameter file. If FROM_VEGHIST = Use the ALBEDO values from the veg_hist forcing files. Note: for this to work, ALBEDO must be supplied in the veg_hist files and listed in the global parameter file as one of the variables in the files. <br><br>Default = FROM_VEGLIB.  |
-| LAI_SRC            | string | N/A           | This option tells VIC where to look for LAI values: FROM_VEGLIB = Use the LAI values listed in the vegetation library file.FROM_VEGPARAM = Use the LAI values listed in the vegetation parameter file. Note: for this to work, VEGPARAM_LAI must be TRUE.FROM_VEGHIST = Use the LAI values listed in the veg_hist forcing files. Note: for this to work, LAI_IN must be supplied in the veg_hist files and listd in the global parameter file as one of the variables in the files. Default = FROM_VEGLIB.                                                                                                                                                                |
+| LAI_SRC            | string | N/A           | This option tells VIC where to look for LAI values: FROM_VEGLIB = Use the LAI values listed in the vegetation library file.FROM_VEGPARAM = Use the LAI values listed in the vegetation parameter file. Note: for this to work, VEGPARAM_LAI must be TRUE.FROM_VEGHIST = Use the LAI values listed in the veg_hist forcing files. Note: for this to work, LAI must be supplied in the veg_hist files and listd in the global parameter file as one of the variables in the files. Default = FROM_VEGLIB.                                                                                                                                                                |


Add space in the middle of TRUE.FROM_VEGHIST

jhamman · 2017-12-19T21:30:16Z

vic/drivers/shared_image/src/vic_init.c

+                else {
+                    log_warn("Sum of the snow band area fractions is 0, "
+                             "setting first fraction to 1\n%s", locstr);
+                    soil_con[i].AreaFract[0] = 1.;


I think if there were no fractions in the elevation dimension, then we should raise an error, no?

…n snow band areas sum to 0.

tbohn · 2018-01-10T22:22:18Z

Also, what type of comments should I add to ReleaseNotes.md? Should I describe all the code changes (briefly of course) or just the changes to the user interface (changed name of some variables/options)?

jhamman · 2018-01-10T22:29:23Z

I'll leave the content in the release note up to you. Look at what we wrote leading up to 5.0.

tbohn · 2018-01-11T02:39:31Z

Running uncrustify produces many changes, to many files that I didn't even make changes to. And I'm not thinking you'll like the types of format changes it makes. For example, from diffing the after and before, we can see:
diff -r VIC.uncrustify/vic/drivers/cesm/src/vic_cesm_init_library.c VIC/vic/drivers/cesm/src/vic_cesm_init_library.c
183c183
< l2x_vic[i].l2x_Fall_lwup = -1 *param.EMISS_GRND *CONST_STEBOL *pow(
---
> l2x_vic[i].l2x_Fall_lwup = -1 * param.EMISS_GRND * CONST_STEBOL * pow(

so, uncrustify, at least on my machine, is making multiplication look like pointer dereferencing.

I ran uncrustify via the run_uncrustify.bash script.

… into feature/clean_up_vic_run

tbohn · 2018-01-11T20:54:21Z

I've found a potential bug in my changes - nothing major; can fix it by moving a block of code. But please wait before merging this PR until I've properly fixed the problem.

jhamman · 2018-01-17T17:51:49Z

@tbohn - is this ready to go?

tbohn · 2018-01-17T21:09:59Z

yes, this is ready.

jhamman · 2018-01-19T05:07:29Z

@yixinmao - can you look into why these tests are failing now? Is it a Python library version issue?

yixinmao · 2018-01-25T22:49:30Z

@tbohn : I briefly looked at the test results, and it seems like for the classic driver, there are some nan's in the flux output files; and with FULL_ENERGY=TRUE and FROZEN_SOIL=TRUE, it seems like there are some issue with iteration convergence so that the run doesn't end (at least not for a long time). These issues occur locally (so not a travis thing). Do you have any idea what are the causes?

tbohn · 2018-01-26T07:46:03Z

@yixinmao : strange, no, I don't know what is causing those NaNs. The only things I did were to (a) merge in the latest develop changes and (b) move a block of code to a different place in vic_run.c. It is, of course, possible that I caused the NaNs by doing so, although in my tests on my own machine those didn't occur. I'll take a look.

yixinmao · 2018-01-26T18:20:52Z

@tbohn hmm weird. Did all tests pass on your machine? They do fail on mine the same way that travis tests fail (i.e., system test failure as well as out of time). Let me know what you find (or can't find).

tbohn · 2018-01-26T19:04:23Z

@yixinmao I wasn't referring to travis tests on my system; I was referring to tests at my own test sites. Another shortcoming of my tests, which is completely my fault, is that I was using a copy of my git repository for dev/testing, and then ported the changes into git. So there's a chance that the testing version of the code was fine but my port somehow messed things up.

yixinmao · 2018-01-26T19:22:34Z

@tbohn OK. The tests failed on my local machine without travis (I fetched your branch directly). Do you think you could take a look to see what might be going on? It's probably faster than me looking from scratch. But if you still can't find anything, let me know and I can take a closer look.

tbohn · 2018-01-26T19:25:03Z

@yixinmao yes, I will take a look this afternoon - I don't expect you to do that investigation; it's my responsibility to debug it, I think. I only needed help earlier in understanding the nature of the travis errors (the messages were not clear to me).

…ion.

yixinmao · 2018-02-03T01:23:45Z

@jhamman : after @tbohn fixes all tests passed and this PR is ready to be merged.

jhamman · 2018-02-03T02:52:37Z

This looks good to me but I'd like @bartnijssen to give it a final look before merging.

jhamman requested a review from dgergel July 12, 2017 19:03

dgergel added the vic_run label Jul 12, 2017

tbohn added 2 commits July 12, 2017 13:14

Cleanup after merging with develop, which seemed to have removed my c…

f555551

…hanges.

Simplified prepare_full_energy() to operate on a single instance of c…

acb99ae

…ell_data and energy_bal structs (removed loop over bands).

tbohn force-pushed the feature/clean_up_vic_run branch from c857d47 to acb99ae Compare July 12, 2017 21:13

tbohn commented Jul 12, 2017

View reviewed changes

tbohn and others added 10 commits July 17, 2017 15:44

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

tbohn mentioned this pull request Dec 18, 2017

Develop #750

Merged

10 tasks

tbohn changed the title ~~Feature/clean up vic run~~ Miscellaneous clean-up and minor fixes. Dec 18, 2017

ted added 3 commits December 18, 2017 15:27

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

This was referenced Dec 19, 2017

Cleanup: Change netcdf names for vegetation forcing variables fcanopy and albedo #764

Closed

Cleanup: Change forcing variable LAI_IN to LAI #763

Closed

jhamman reviewed Dec 20, 2017

View reviewed changes

ted added 2 commits December 19, 2017 20:23

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

jhamman requested a review from yixinmao January 2, 2018 05:24

ted added 2 commits January 10, 2018 20:15

Added release notes entry.

7e87209

Merge branch 'feature/clean_up_vic_run' of https://github.com/tbohn/VIC…

9aa3ac6

… into feature/clean_up_vic_run

Fixed bug in location of conversion from global to local LAI etc.

d3d5e17

Merge branch 'develop' into feature/clean_up_vic_run

e505692

ted added 2 commits February 1, 2018 09:18

Merge branch 'develop' into feature/clean_up_vic_run

9131e3e

Fixed problem with nans caused by location of global to local convers…

40bd771

…ion.

tbohn mentioned this pull request Feb 3, 2018

Feature/clean up esoil #777

Merged

6 tasks

yixinmao approved these changes Feb 3, 2018

View reviewed changes

bartnijssen approved these changes Feb 3, 2018

View reviewed changes

bartnijssen merged commit c7b369c into UW-Hydro:develop Feb 3, 2018

tbohn deleted the feature/clean_up_vic_run branch March 8, 2018 06:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Miscellaneous clean-up and minor fixes. #723

Miscellaneous clean-up and minor fixes. #723

tbohn commented Jul 11, 2017 •

edited

Loading

jhamman commented Jul 12, 2017

tbohn commented Jul 12, 2017

dgergel commented Jul 12, 2017

tbohn Jul 12, 2017

jhamman left a comment

jhamman Dec 19, 2017

tbohn Dec 20, 2017

jhamman Dec 19, 2017

tbohn Dec 20, 2017

jhamman Dec 19, 2017

tbohn Dec 20, 2017

tbohn commented Jan 10, 2018

jhamman commented Jan 10, 2018

tbohn commented Jan 11, 2018

tbohn commented Jan 11, 2018

jhamman commented Jan 17, 2018

tbohn commented Jan 17, 2018

jhamman commented Jan 19, 2018

yixinmao commented Jan 25, 2018

tbohn commented Jan 26, 2018

yixinmao commented Jan 26, 2018

tbohn commented Jan 26, 2018

yixinmao commented Jan 26, 2018

tbohn commented Jan 26, 2018

yixinmao commented Feb 3, 2018

jhamman commented Feb 3, 2018

Miscellaneous clean-up and minor fixes. #723

Miscellaneous clean-up and minor fixes. #723

Conversation

tbohn commented Jul 11, 2017 • edited Loading

jhamman commented Jul 12, 2017

tbohn commented Jul 12, 2017

dgergel commented Jul 12, 2017

tbohn Jul 12, 2017

Choose a reason for hiding this comment

jhamman left a comment

Choose a reason for hiding this comment

jhamman Dec 19, 2017

Choose a reason for hiding this comment

tbohn Dec 20, 2017

Choose a reason for hiding this comment

jhamman Dec 19, 2017

Choose a reason for hiding this comment

tbohn Dec 20, 2017

Choose a reason for hiding this comment

jhamman Dec 19, 2017

Choose a reason for hiding this comment

tbohn Dec 20, 2017

Choose a reason for hiding this comment

tbohn commented Jan 10, 2018

jhamman commented Jan 10, 2018

tbohn commented Jan 11, 2018

tbohn commented Jan 11, 2018

jhamman commented Jan 17, 2018

tbohn commented Jan 17, 2018

jhamman commented Jan 19, 2018

yixinmao commented Jan 25, 2018

tbohn commented Jan 26, 2018

yixinmao commented Jan 26, 2018

tbohn commented Jan 26, 2018

yixinmao commented Jan 26, 2018

tbohn commented Jan 26, 2018

yixinmao commented Feb 3, 2018

jhamman commented Feb 3, 2018

tbohn commented Jul 11, 2017 •

edited

Loading