Get rid of user_def.h

[jhamman]

Description - Either remove these options or move them to global parameter file. Remove #ifdef’s from code. Remove VIC’s –o flag. Optional? - No

[bartnijssen]

Ted to scope out what is easy to remove (these will become part of 4.2) and what is hard (VIC 5.0). This issue will likely be split into multiple new issues (at which time this one can be closed).

[tbohn]

Here are my recommendations. In all cases, we can do something for 4.2, so that user_def.h potentially could be removed for 4.2. My recommendations are for 4.2 unless otherwise stated.

Option	Description	Removal	Recommendation
VERBOSE	Enables some print statements. Not consistently applied through code; most print statements are independent of VERBOSE.	Easy	Remove (easy; 4.2) or extend throughout code for consistency and convert to run-time (hard; 5.0)
CLOSE_ENERGY	Controls iteration between surface and canopy energy balances.	Hard (unless removing the iteration altogether)	Convert to run-time option
QUICK_FS, QUICK_FS_TEMPS	Enables linear approximation to compute maximum unfrozen water content look-up table. This speeds up frozen soil runs, but may introduce new errors. Apparently it has never been completely tested.	Easy (limited to a few functions)	Remove (if we don't think anyone will use it) or convert to run-time
LOW_RES_MOIST	Uses formulation of Boone and Wetzel (1996) for vertical drainage (linear interpolation of logarithm of matric potential) instead of computing hydraulic conductivity. This mainly appears in runoff.c.	Easy	Keep and convert to run-time
NO_REWIND	Controls rewinding of input files. Re-reading files from beginning adds time to the run. If files are in exactly the same order as the soil parameter file, rewinding is not necessary.	Easy	Either hard-code this behavior and require that all input files have exact same set of cells in same order, or convert to run-time option
OUTPUT_FORCE	Switches VIC from hydrologic model to forcing disaggregator.	Hard	For 4.2, convert to run-time. When a stand-alone disaggregator has been set up (VIC 5.0?) remove this option.
OUTPUT_FORCE_STATS	Computes/outputs statistics of forcings, for the purpose of debugging/quality control.	Easy	Remove
SPATIAL_FROST, FROST_SUBAREAS	Enables uniform distribution of soil T about its mean.	Hard (in many functions); also, this is in use.	Convert to run-time
SPATIAL_SNOW	Enables uniform distribution of snow depth about its mean (during melt).	Easy; but this is in use.	Convert to run-time
EXCESS_ICE	Enables simulation of excess ground ice. Porosity and other soil parameters that are affected by excess ice become dynamic. Primarily affects: full_energy.c, frozen_soil.c, initialize_model_state.c, read_soilparam.c, runoff.c, soil_conduction.c.	Easy (borderline); probably not in use	Remove
MAX_VEG, MAX_LAYERS, etc	Array limits for allocation purposes.	Hard (have to come up with method to replace these)	Move to vicNl_def.h
MAXIT_FE	Max iterations for surface energy balance	N/A	Move to vicNl_def.h
LAI_WATER_FACTOR	Factor relating LAI to canopy interception storage capacity.	N/A	Move to vicNl_def.h
LWAVE_COR	Correction factor to apply to computed incoming longwave radiation, for the purposes of bias correction. Only used in calc_longwave.c.	Easy	Remove (given good performance of LW algorithm demonstrated in Bohn et al 2013)

[tbohn]

Note: the contents of snow.h could also potentially be moved to vicNl_def.h; the contents of LAKE.h could be moved to vicNl.h; and some/all of the contents of global.h could be moved to vicNl_def.h. This is mainly for ease of dealing with the code.

[bartnijssen]

Ease is good. However, if they are functionally different pieces of the code it might be good to keep them separate. Originally snow.h was separate, because it was basically the same snow code as DHSVM and it made it easier to upgrade compare. These are a bit different than the user_def.h cases and I think a case could be made to keep them separate.

Bart

On Dec 18, 2013, at 10:12 AM, Ted Bohn notifications@github.com wrote:

Note: the contents of snow.h could also potentially be moved to vicNl_def.h; the contents of LAKE.h could be moved to vicNl.h; and some/all of the contents of global.h could be moved to vicNl_def.h. This is mainly for ease of dealing with the code.

— Reply to this email directly or view it on GitHub.

[tbohn]

If there are no objections to my recommendations, I can go ahead and start making those changes.

Let me know if you would like me to break any of the individual options out into separate issues (and assign them to myself).

[bartnijssen]

As mentioned - I am not sure that we want to get rid of snow.h at this time, but please proceed with user_def.h.

On Dec 22, 2013, at 3:41 PM, Ted Bohn notifications@github.com wrote:

If there are no objections to my recommendations, I can go ahead and start making those changes.

Let me know if you would like me to break any of the individual options out into separate issues (and assign them to myself).

— Reply to this email directly or view it on GitHub.

[jhamman]

@tbohn, at a minimum, lets try to put each change to user_def.h variables in their own commits. They all can live under this issue.

[tbohn]

A couple questions:

1. Joe and Bart, before I remove QUICK_FS, is there any chance you'd want to use this feature in your RACM project? I know you want to simulate frozen soil and that runtime is an issue. Keep in mind that this feature has never been fully tested (according to the feature's own comment statements). So, to use it, I guess we'd need to test it to verify whether it works.
2. Regarding VERBOSE: another possible solution could be to keep it (as a run-time option, rather than compile-time) and move it into a wrapper function (similar to nrerror) that checks its value before printing. The hard part would be going through the code and replacing all printing statements with a single function. This might be worth breaking out into a separate issue, that includes reconciling vicerror() and nrerror(), perhaps creating a single printing function that handles all "log" output?

[bartnijssen]

1. I think it QUICK_FS can be removed, especially if it has not been tested. We are not using it right now and runtime is not really an issue for our coupled arctic modeling since we run on many processors. We are not using it in the current version that is coupled in RASM.
2. Yes - I would break it into a separate issue. We'd probably want some log function that uses a log level that can be set by the use at run-time to determine the verbosity for the amount of detail to be logged. For example, analogous to pythons logging module (http://docs.python.org/2/library/logging.html):

CRITICAL 50 ERROR 40 WARNING 30 INFO 20 DEBUG 10

Bart

On Dec 26, 2013, at 4:50 PM, Ted Bohn notifications@github.com wrote:

A couple questions:

1. Joe and Bart, before I remove QUICK_FS, is there any chance you'd want to use this feature in your RACM project? I know you want to simulate frozen soil and that runtime is an issue. Keep in mind that this feature has never been fully tested (according to the feature's own comment statements). So, to use it, I guess we'd need to test it to verify whether it works.
2. Regarding VERBOSE: another possible solution could be to keep it (as a run-time option, rather than compile-time) and move it into a wrapper function (similar to nrerror) that checks its value before printing. The hard part would be going through the code and replacing all printing statements with a single function. This might be worth breaking out into a separate issue, that includes reconciling vicerror() and nrerror(), perhaps creating a single printing function that handles all "log" output?

— Reply to this email directly or view it on GitHub.

[tbohn]

OK, how about this: I will handle VERBOSE in 2 stages. The first stage will simply be to transfer the VERBOSE precompile definition from user_def.h to vicNl_def.h, so that user_def.h can be removed and this issue can be closed. For the second stage, I'll create a separate issue to create consistent logging. VERBOSE will go away as part of the second stage. Any objections?

[bartnijssen]

No objections

On Dec 27, 2013, at 4:31 PM, Ted Bohn notifications@github.com wrote:

OK, how about this: I will handle VERBOSE in 2 stages. The first stage will simply be to transfer the VERBOSE precompile definition from user_def.h to vicNl_def.h, so that user_def.h can be removed and this issue can be closed. For the second stage, I'll create a separate issue to create consistent logging. VERBOSE will go away as part of the second stage. Any objections?

— Reply to this email directly or view it on GitHub.

[tbohn]

One last question. Regarding NO_REWIND, I suggested two options: either move it to the options_struct in vicNl_def.h as a run-time option, or getting rid of it altogether. I'm leaning towards getting rid of it, since it seems like our parameter files are always set up so that all grid cells are present and appear in the same order. It would just fall on the user to set up their files correctly. Any objections?

[bartnijssen]

As long as the program aborts if it cannot find the next grid cell that is fine with me. I'd like to move my parameter files to netcdf anyway (eventually), where this would not be an issue. We may also write a little preprocessir script that properly checks (and reorders the files if need be).

Bart

On Dec 28, 2013, at 12:23 PM, Ted Bohn notifications@github.com wrote:

One last question. Regarding NO_REWIND, I suggested two options: either move it to the options_struct in vicNl_def.h as a run-time option, or getting rid of it altogether. I'm leaning towards getting rid of it, since it seems like our parameter files are always set up so that all grid cells are present and appear in the same order. It would just fall on the user to set up their files correctly. Any objections?

— Reply to this email directly or view it on GitHub.

[tbohn]

These changes are now complete. User_def.h is now gone (for details, see my pull request). I am creating 2 new issues (each one much smaller than this one was):

• Consistent logging function (to replace the VERBOSE option and nerror and vicerror functions) (issue #64)
• Consider removing the LOG_MATRIC option (was LOW_RES_MOIST) - preliminary tests indicate that it has very little effect (issue #65)

[tbohn]

This code update also addresses issue #38.

[tbohn]

Now that the pull request has been executed, I'm closing this issue.