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.

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

Routing extension for VIC image driver. #231

Merged
merged 157 commits into from
Oct 20, 2017

Conversation

wietsefranssen
Copy link
Contributor

@wietsefranssen wietsefranssen commented Jul 30, 2015

Affected files in the original VIC files are: ./drivers/image/Makefile and ./drivers/image/src/vic_image.c.

To change the routing scheme once they are implemented, only change the following line in the makefile
-> ROUT=rout_stub

Still only an empty routing module is implemented (rout_stub). See ./extensions/rout_stub

closes #24

Affected files in the original VIC files are: './drivers/image/Makefile'  and './drivers/image/src/vic_image.c'.

To change the routing scheme once they are implemented, only change the following line in the makefile
-> ROUT=rout_stub

Still only an empty routing module is implemented (rout_stub). See ./extensions/rout_stub
…e/routing_lohmann

* 'develop' of https://github.com/UW-Hydro/VIC:
  update prefix string allocation size
  Cleanup ascii output file header. The major change here is that the header does not include a comment for the line containing the variable names.
  move out_data_file_struct and Error_struct to shared driver header (from vic_run)
@bartnijssen
Copy link
Member

@jhamman : I'll let you go through this first. Let me know when I should take a look. Mare sure that code is formatted correctly (uncrustify)

#ifndef ROUT_STUB_H
#define ROUT_STUB_H

/******************************************************************************
Copy link
Member

Choose a reason for hiding this comment

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

Let's include this line here as well:

#define ROUT_EXT "rout_stub"

This will give any routines using rout.h some knowledge about which extension is active. This will become more important when you get to the other extensions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I will do that

@jhamman
Copy link
Member

jhamman commented Jul 30, 2015

Everything looks good so far here. There are a few small fixes. Starting with your next commit, run uncrustify on all the files you have changed before you commit.

@bartnijssen
Copy link
Member

When running uncrustify use the configuration file in the repo under tools/code_format. That directory also includes a readme with instructions for running uncrustify.

…e/routing_lohmann_dev

# By Joe Hamman (82) and others
# Via Joe Hamman (22) and Bart Nijssen (16)
* 'develop' of https://github.com/UW-Hydro/VIC: (90 commits)
  Fix links for forcing input documentation
  Update FAQ about running on OS X and tone down the CAPS SHOUTING
  remove other unused declarations
  remove find_average_layer from header files
  fixing up UW-Hydro#258
  Updates to Model Overview and References * Clarified descriptions for partial veg cover fraction and time-varying veg parameters. * Added references to Bohn and Vivoni (2015). * Added schematic of partial veg cover fraction. * Numbered all figures in the model overview. * Clarified descriptions of a few other features. * Added references to Francini and Pacciani (1991) for soil evaporation and ARNO baseflow; added reference to Brooks and Corey (1964) for gravity-driven drainage. * Added links from all references to the References.md page where links were not present. * Added references to Bohn and Vivoni (2015), Francini and Pacciani (1991), and Brooks and Corey (1964) to the References.md page. * Alphabetized references on the References.md page.
  Rewrite of the VIC front page README
  update links in readme to new website
  dont put badge in readme header
  clean up readme files a bit
  add comment to python driver setup.py about logging
  add doxygen config file
  add readme to docs dir
  cleanup mkdocs.yml
  Removed docs/Development/ModelDevelopment.md from mkdocs.yml
  Minor edits to documentation
  set log level in setup.py
  fix a few bugs in vic_time.c, add unit tests for main functions
  update outline levels in model overview
  remove goes radiation doc link (since it doesn't exisit)
  ...

Conflicts:
	vic/drivers/image/Makefile
…e/routing_lohmann_dev

* 'develop' of https://github.com/UW-Hydro/VIC: (51 commits)
  update git ignore
  fix bash syntax
  debugging multi os python build
  update python miniconda for osx and linux
  updates to python builds, including osx builds for all python tests
  update nc and mpi paths in image driver build
  build image driver on osx
  move out_data structures to shared driver
  move zero_output_list to driver/shared
  remove brew doctor
  brew doctor
  debugging homebrew install
  print path in classic driver builds
  update osx build
  fix macos build
  update macos build
  update mac os build
  add brew_installs functionality
  travis update
  simplier macos build for classic
  ...
*Made mapping: rout-outlet-index to VIC-index
*Filled: OUT_DISCHARGE variable
@bartnijssen
Copy link
Member

@wietsefranssen : Hi Wietse - can you give an update one where things are at?

@wietsefranssen
Copy link
Contributor Author

Sorry for the long quietness!
You will hear from me next Monday or Tuesday.

…e/routing_lohmann_dev

* 'develop' of https://github.com/UW-Hydro/VIC:
  Made the reference to Liang et al (1994) in readme.md more explicit.
  Moved ATBD reference into main references section.  Emphasized Liang et al 1994 as primary reference.
  refactor comparisons to use type separate functions
  remove unused measure_h
  update comparisons module
  initial commit for VIC.5 docs update
  fix warnings on soome compilers that localtime_r is declared implicitly
  add comparisons module
  remove log_ncerr macro and replace with log_err, added helpful error messages
  fix zero dim array error raised by PGI compilers
  fix failing test for python 3, decode bytestring
  fix typo in vic_mpi_support.c
  previous commit fixes know failing test
  fix log to file in image driver when running mpi, now brodcasting filenames struct
outlet index of VIC gridcell check
@wietsefranssen
Copy link
Contributor Author

wietsefranssen commented Sep 19, 2017

@jhamman @bartnijssen.
We separated the state restore (and state save) functions as suggested by @bartnijssen (Mar 4 2017) in order to avoid having the variable 'STATE_ROUT_RING' in the statefile while 'ROUT_STUB' is enabled (no routing).
Also 'statefile.md' and 'ReleaseNotes.md' have been adapted.

The Output state variables (enum) are defined in 'vic_driver_shared_all.h'. This headerfile will be compiled by all drivers. This means that we needed to find a way to only include 'STATE_ROUT_RING' when the extension RVIC is enabled ('N_STATE_VARS' should be equal to the exact number of state variables that are written to the statefile). We experimented with various options including moving 'vic_driver_shared_all.h' to the extension folders. However this caused compilation issues for the other drivers. The solution we implemented is using pre-processor directive '#ifdef EXTENSION_RVIC' in 'vic_driver_shared_all.h'. Note that the definition of 'EXTENSION_RVIC' cannot be placed in 'rout.h' because 'vic_driver_shared_all.h' needs to be called first. Therefore we added a header file 'image_extension_name.h' with a definition of the extension name in every extension (eg. '#define EXTENSION_STUB' for the STUB extension).

@jhamman
Copy link
Member

jhamman commented Sep 19, 2017

@wietsefranssen - it looks like a few files (vic_restore.c, vic_store.c) may have been accidentally moved to the routing extension. Can you take a look into that and see if that can be fixed.

I think we can avoid any #ifdef preprocessor calls in the c code but we'll wait to look at that until after the issue described above is fixed.

@wietsefranssen
Copy link
Contributor Author

@jhamman @bartnijssen
We did a first attempt to move the vic_store.c back to its original position. Please have a look if this is the way forward. If this is the case we will proceed with vic_restore.c.

ymao and others added 2 commits October 17, 2017 15:17
Final fixes for the routing extension
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.

I had just a few minor semanitc comments but this looks ready to go. Thanks @wietsefranssen
and @isupit for sticking with this. Thanks @yixinmao for helping bring this across the finish line.

@@ -107,6 +107,10 @@ To check which release of VIC you are running:

The VIC image and CESM drivers now may be optionally compiled with OPENMP to enable shared memory thread parallelization. This option should improve the parallel scaling of these drivers by reducing the number of MPI messages and increasing message size.

8. Added routing extensions ROUT_STUB and ROUT_RVIC for the VIC image driver ([GH#231](https://github.com/UW-Hydro/VIC/pull/231))

The VIC image driver can be optionally compiled with ROUT_RVIC to enable routing in image mode (ROUT_STUB is the default extension which means no routing). With ROUT_RVIC enabled, the output variable ``OUT_DISCHARGE`` is available, and there will also be an extra state variable ``STATE_ROUT_RING`` stored in the state file.
Copy link
Member

Choose a reason for hiding this comment

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

insert "streamflow" before "routing" here and in line 110.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

# |---------- |--------------------------------- |
# | rout_stub | Stub routing model (no routing) |
# | rout_rvic | Use RVIC routing scheme |
ROUT=rout_stub
Copy link
Member

Choose a reason for hiding this comment

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

should this be ROUT := rout_stub to allow it to be set on the command line via make ROUT=rout_rvic?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ROUT=rout_rvic is also working with ROUT=rout_stub, but for future additions it would probably better to use your suggestion. We included it.

@@ -27,6 +27,7 @@
#ifndef VIC_DRIVER_SHARED_IMAGE_H
#define VIC_DRIVER_SHARED_IMAGE_H

#include <image_extension_name.h>
Copy link
Member

Choose a reason for hiding this comment

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

should we call this rout_extension_name.h?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This indeed streamlines better with @bartnijssen latest additions.
Done.

@@ -66,7 +66,7 @@
#define check_nc_status(A, M, ...) if (A != NC_NOERR) {log_ncerr(A, M, \
## __VA_ARGS__); \
errno = 0; exit( \
EXIT_FAILURE);}
EXIT_FAILURE); }
Copy link
Member

Choose a reason for hiding this comment

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

is this change required?

Copy link
Contributor

Choose a reason for hiding this comment

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

This change was done by run_crustify

@@ -882,6 +883,9 @@ vic_restore(void)
}
}

// routing ring
vic_restore_extension(&(filenames.init_state), state_metadata);
Copy link
Member

Choose a reason for hiding this comment

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

rename function to vic_restore_rout_extension

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done. also changed vic_store_extension into vic_store_rout_extension

@@ -1269,6 +1270,10 @@ vic_store(dmy_struct *dmy_state,
}
}

// store extension variables
vic_store_extension(&nc_state_file);
Copy link
Member

Choose a reason for hiding this comment

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

rename to vic_store_rout_extension

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done (see above sorry ;) )

@@ -1324,9 +1329,13 @@ set_nc_state_file_info(nc_file_struct *nc_state_file)
nc_state_file->time_size = NC_UNLIMITED;
nc_state_file->veg_size = options.NVEGTYPES;

// set ids and dimension sizes of the extension variables
set_nc_state_file_info_extension(nc_state_file);
Copy link
Member

Choose a reason for hiding this comment

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

rename to set_nc_state_file_info_rout_extension

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

if (options.LAKES) {
status = nc_def_dim(nc_state_file->nc_id, "lake_node",
nc_state_file->lake_node_size,
&(nc_state_file->lake_node_dimid));
check_nc_status(status, "Error defining lake_node in %s", filename);
}

// add extension dimensions
initialize_state_file_extension(filename, nc_state_file);
Copy link
Member

Choose a reason for hiding this comment

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

rename to inialize_state_file_rout_extension

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@yixinmao
Copy link
Contributor

Thanks @jhamman and @bartnijssen for review. Also, another thing to update before merging this PR is to sync with the latest develop branch.

@bartnijssen bartnijssen merged commit f76d299 into UW-Hydro:develop Oct 20, 2017
@bartnijssen
Copy link
Member

VICTORY 🎉

@wietsefranssen
Copy link
Contributor Author

GREAT!! Thanks to all!

@wietsefranssen
Copy link
Contributor Author

@ALL We included the changes suggested by @jhamman. and made a new pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants