Adds CN to classic driver. Still includes verbose option.

[jhamman]

Initial pull request for adding CN to VIC.

What are our (@bartnijssen and @mabrunke) thoughts on:

• Basic implementation,
• Portability,
• Documentation,
• Location of CN/src code, and
• Merging recent cleanup changes to the feature/CN branch.

[bartnijssen]

Do we want to pull this directly into develop? I'd rather have a separate branch off develop into which we can merge this, so that develop remains a clean path to VIC 5

[mabrunke]

Bart:

I think a separate branch would be good for now at least until VICCN is a
little further advanced.

--Michael

Do we want to pull this directly into develop? I'd rather have a separate
branch off develop into which we can merge this, so that develop remains a
clean path to VIC 5

---

Reply to this email directly or view it on GitHub:
#174 (comment)

[jhamman]

Agreed. Once, I have the drvier/RASM branch in a more stable state, we can edit this PR so that it points to that branch.

[jhamman]

Ok, I just finished a first read through the code and here are some initial comments:

Logistics:

• Update branch using current develop branch. feature/CN branch is now quite a ways out of date with thedevelop branch. The update/merge will require a bit of work but will make comparing to the develop or driver/RASM branches much easier.
• After the merge, the following tasks will need to be completed:
  • Convert "#define" variable names to enums where applicable (see Cumbersome input and output variable declarations in vicNl_def.h #36 and replace #define statments for options and variable ids with enums #175). This applies to "Carbon model choices" and "Output Variable Types".
  • Use logging functions rather than print statements.
  • Add documentation to function and structure members (see example in Code cleanup checklist #162)

Code Review/Style/Comments:

• We should think about how the C/Fortran interface looks. vic2clmtype_() takes 192 arguments which screams fragile. @bartnijssen and Tony Craig may have suggestions on how to do this.

• Handle error codes. For example, here in vic_run(),

  if(options.CARBON == CN_NORMAL || options.CARBON == CN_ADECOMP) {
      ErrorFlag = VICCNInterface(rec, Nveg, npfts, dmy, gp, atmos, all_vars, soil_con, veg_con, veg_lib, cn);
  ...

  Once the branch is updated to the current develop branch, log_err() will be available allowing:

  if (!ErrorFlag) {
      log_err("Error calling VICCNInterface");
  }

• Variable types and memory usage. cn_data_struct is quite large and includes only double precision variables.

  • What is 21 (e.g. onset_flag[21])? It would probably be better use a global size variable #define MAX_CN_LAYERS 21 and onset_flag[MAX_CN_LAYERS]
  • Use integer or bool datatypes for flags and counters (e.g. double onset_flag[21]; --> bool onset_flag[21];

• Move "Record Carbon Cycling Variables" out of "Collect Water Balance Terms" function in put_data.c. Rather than putting this code in an existing function, its probably worth creating a collect_carbon_terms() function that is only called when CARBON == TRUE.

• Remove hardcoded paths from Makefile

• Remove verbose comments

• Remove hard coded literals when possible from cn/src/VICCNInterface.c. Physical constants are in vic_run/include/physical_constants.h

Relevant Upcoming Changes:

• Timestep units in global_param.dt are changing from hours to seconds.
• MPI in driver/image. @bartnijssen can comment on which, if any changes will be necessary in this regard.

Next Steps:

Once we've settled on the changes to the classic driver, the next step is to port to the image mode and RASM drivers.

[tbohn]

I agree with moving "collect carbon terms" into its own function - I should
have done it that way to begin with!

On Fri, Jan 9, 2015 at 4:07 PM, Joe Hamman notifications@github.com wrote:

Ok, I just finished a first read through the code and here are some
initial comments:
Logistics:

• Update branch using current develop branch. feature/CN branch is now
  quite a ways out of date with thedevelop branch. The update/merge will
  require a bit of work but will make comparing to the develop or
  driver/RASM branches much easier.
• After the merge, the following tasks will need to be completed:
  • Convert "#define" variable names to enums where applicable (see
    Cumbersome input and output variable declarations in vicNl_def.h #36 Cumbersome input and output variable declarations in vicNl_def.h #36 and replace #define statments for options and variable ids with enums #175
    replace #define statments for options and variable ids with enums #175). This applies to
    "Carbon model choices" and "Output Variable Types".
  • Use logging functions rather than print statements.
  • Add documentation to function and structure members (see example
    in Code cleanup checklist #162 Code cleanup checklist #162)

Code Review/Style/Comments:

• We should think about how the C/Fortran interface looks.
  vic2clmtype_() takes 192 arguments which screams fragile. @bartnijssen
  https://github.com/bartnijssen and Tony Craig may have suggestions

  on how to do this.

  Handle error codes. For example, here in vic_run()
  https://github.com/mabrunke/VIC/blob/feature/CN/vic_run/src/vic_run.c#L370-L374
  ,

  if(options.CARBON == CN_NORMAL || options.CARBON == CN_ADECOMP) {
  ErrorFlag = VICCNInterface(rec, Nveg, npfts, dmy, gp, atmos, all_vars, soil_con, veg_con, veg_lib, cn);
  ...

  Once the branch is updated to the current develop branch, log_err()
  will be available allowing:

  if (!ErrorFlag) {
  log_err("Error calling VICCNInterface");
  }

  Variable types and memory usage. cn_data_struct is quite large and
  includes only double precision variables.

  • What is 21 (e.g. onset_flag[21])? It would probably be better use a
    global size variable #define MAX_CN_LAYERS 21 and
    onset_flag[MAX_CN_LAYERS]
    • Use integer or bool datatypes for flags and counters (e.g. double
      onset_flag[21]; --> bool onset_flag[21];
  • Move "Record Carbon Cycling Variables" out of "Collect Water
    Balance Terms" function in put_data.c. Rather than putting this code
    in an existing function, its probably worth creating a
    collect_carbon_terms() function that is only called when CARBON == TRUE
    .

• Remove hardcoded paths from Makefile

• Remove verbose comments

• Remove hard coded literals when possible from cn/src/VICCNInterface.c.
  Physical constants are in vic_run/include/physical_constants.h

Relevant Upcoming Changes:

• Timestep units in global_param.dt are changing from hours to seconds.
• MPI in driver/image. @bartnijssen https://github.com/bartnijssen
  can comment on which, if any changes will be necessary in this regard.

Next Steps:

Once we've settled on the changes to the classic driver, the next step is
to port to the image mode and RASM drivers.

—
Reply to this email directly or view it on GitHub
#174 (comment).

[bartnijssen]

Some more comments with respect to organization (I haven't gone through the actual code yet):

• Cleanly separate the CLM code that will be imported as a "block" or module (i.e. we will make no changes inside this) from the wrapper around it that will let this module be called from VIC.
• Separate all the I/O from the physics (akin to vic_run() and the different drivers). We should be able to use the CN code with all the drivers (even if we originally implement it for only a subset), without any changes to the physics code.
• Determine where all the pieces will live within the vic repo directory structure. In VIC5 this will have to be broken out into separate pieces rather than as 60+ files in cn/src

[jhamman]

@mabrunke - I'm going to close this PR. Can you address the issues that @bartnijssen and I have brought up and open a new PR against this branch: https://github.com/UW-Hydro/VIC/tree/driver/cesm.

Also, while you are moving things around a bit, we have reorganized the VIC directory structure a bit to better support adding extensions into VIC, such as CN. This PR is a good example of how we're structuring things: #231. Following how the extension was added in #231, you'll want to add two extensions:

• dynveg_stub
• synveg_cn