Added ability to convert VIC 4.2 ascii state files to VIC 5.0 netcdf #43

tbohn · 2016-03-17T23:37:46Z

Added ability to convert ascii state files to netcdf.

Addresses issue #35.

…ne header

…nced with large datasets, binary output files are not supported in this version

merge develop into master for tonic 0.1

1. optional extra columns in soil_param file 2. optional extra fcanopy in veg_lib file 3. optional blowing_snow params in veg_param file 4. optional lai, fcanopy, albedo in veg_param file 5. optional lake_param file 6. reading of veg_lib comment field 7. conditional creation of bare soil class, only if not already present

…e_blosnow_veghist Added support for lake, blowing snow, and flexible lai/fcanopy/albedo to grid_params.py

Added support for carbon parameters to grid_params.py

tbohn · 2016-03-19T18:06:21Z

Note: I plan on updating the code in this PR to make it consistent with development I'm currently doing in the VIC code. Some variable names will change and the set of coordinate variables in the output state file will change.

To see example ascii parameter and state files, and the corresponding netcdf parameter and state files produced by this version of tonic, please see:

meter:/raid/tbohn/VIC/test/data/NAM.tonic_test/params:

global.try01 - this is the VIC run that created the ascii state file
soil/veg/lake parameter files listed in global.try01
params.try01.nc - netcdf parameter file output by tonic
state.try01.nc - netcdf state file output by tonic

meter:/raid/tbohn/VIC/test/data/NAM.tonic_test/state:

state.try01_19991231 - ascii state file output by VIC

tbohn · 2016-03-20T00:31:58Z

@jhamman - I see that tonic's write_netcdf() function names the coordinate dimensions 'ni' (for latitude) and 'nj' for longitude in the case of 2d coordinate variables. In this case, the order of the geographic part of the list of dimensions (in the definitions of other variables) is ('nj', 'ni',). Meanwhile, for 1d coordinate variables, tonic names them 'lat' and 'lon' and the order is ('lat', 'lon', ), which is the reverse of the 2d case. Any special reason for this?

VIC (in vic_store()) writes the dimension names as 'ni' and 'nj', and their order is (ny, nx) using the C convention, which is the reverse order of the python convention, so in python, they would be (nx, ny), with y corresponding to latitude and ni, and x corresponding to longitude and nj. So VIC appears to use the same convention as tonic's 2d case. This appears to be true even for VIC's 1d case.

Can I change tonic so that the dimensions are always named 'ni' and 'nj', and always appear in the order ('nj', 'ni', ) regardless of whether they are 1d or 2d?

jhamman · 2016-03-20T16:22:15Z

@tbohn -

The domain dimension convention should be:

1D Coordinates - use lat and lon for both dimension and coordinate names. (see http://cfconventions.org/Data/cf-conventions/cf-conventions-1.6/build/cf-conventions.html#idp5553648)
2D Coordinates - use dimension names that are meaningful (nj, and ni fit) and use 2d coordinate variables (see http://cfconventions.org/Data/cf-conventions/cf-conventions-1.6/build/cf-conventions.html#idp5559280).

I'm not exactly sure what you mean by "C convention" vs. "Python convention" since they should be the same. If we need to switch the dimension order around in tonic or vic_store, that is fine.

tbohn · 2016-03-20T17:10:06Z

Re: C vs Python - never mind. I dimly recall something about the C and Fortran netcdf libraries having opposite array index ordering conventions, but it's irrelevant. I'll just make sure that the arrays coming out of tonic are indexed the same way that VIC expects them to be indexed.

Re: coordinate dimensions and variables: OK, I'll follow the CF convention. I got confused by the 'lats' and 'lons' variables, which actually came from the soil parameter file. Tonic has been outputting both 'lat' and 'lon' 1d variables and the 'lats' and 'lons' 2d variables. Doing so certainly covers our bases, but is that really necessary? Why not just always output 2d coordinate variables called 'lat' and 'lon' (computed via grid params so that they cover cells outside the basin or land mask)? Why output the 'lats' and 'lons' variables from the soil param file (which were the inputs to calc_grid anyway, and therefore redundant)?

tbohn · 2016-03-20T17:14:52Z

(I just now read @jhamman 's comments on UW-Hydro/VIC#367, in which some of my questions here are answered).

tbohn · 2016-03-20T20:59:33Z

Sorry to be such a nitpicker, but for 2d, tonic appears to be inconsistent with vic and CF convention. Here's the code for the 2d case:
else:
f.createDimension('nj', target_grid[XVAR].shape[0])
f.createDimension('ni', target_grid[YVAR].shape[1])
dims2 = ('nj', 'ni', )
coordinates = "{0} {1}".format(XVAR, YVAR)

In vic and the cf convention, i = x (= lon in a regular geographic grid), and j = y (= lat). I think in this case, if vic attempted to read the ni and nj dimensions from the file written by tonic, the grid would be transposed. I'll fix it in tonic.

tbohn · 2016-03-20T21:03:20Z

(and by 'fix', I mean I will swap i and j in the two createDimension calls; everything else will stay the same)

dgergel · 2016-03-22T05:43:01Z

tonic/models/vic/grid_params.py

+    with open(state_file) as f:
+        lines = f.readlines()
+
+    gridcel = np.zeros(cells).astype(int)


Why not set the dtype within np.zeros as gridcel = np.zeros(cells, dtype='int')

No good reason for it - the reason is because I'm still new to python. I'll fix it.

Do this elsewhere as well.

…ro#58)

* update vic_valgrind_suppressions.supp filename * fix another typo in VIC_VALGRIND_SUPPRESSIONS

tbohn · 2017-06-13T23:45:11Z

@jhamman I'm finally working with tonic again, a year later, and I see that this whole line of development that I did never got merged - it's still just hanging there. What do we need to do to get this merged?

It looks like I prepared an update in response to all of the comments raised, but then nothing else has happened. I also see that development has occurred since then, and I'll need to resolve merge conflicts to get this ready for a new pull request.

Any objections to my resolving conflicts and submitting a pull request?

And which branch should I shoot for? I see there's a feature/vic42 branch now. Should this be merged into both vic42 and master?

jhamman · 2017-06-23T17:09:27Z

@tbohn - can you resolve the conflicts here then this can go in?

I'll leave it up to you and the other @UW-Hydro/vic-admins to decide where this should go (master vs. 42)...

Conflicts: forcing_tools/netcdf2vic.py

tbohn · 2017-06-28T20:48:06Z

@UW-Hydro/vic-admins: I think I should merge this into feature/vic42 rather than master, since this is VIC 4.2-specific. Then, we can look into porting it into VIC 5.0.

I have a question about one of the conflicts that I'm trying to merge. feature/vic42/grid_params.py, function grid_params() has an argument of lake_dict=None. In my branch, there is no default for lake_dict. I'm happy to add the lake_dict=None just to eliminate the conflict. However, I see some inconsistencies throughout the code for other similar arguments, with some having default of None, some having default of False, and some having no default.

For now, I'll just resolve the conflict and make a new pull request. But should I open an issue for the inconsistent defaults?

Conflicts: tonic/models/vic/grid_params.py

bartnijssen · 2017-06-29T17:04:45Z

It'd be good to clarify in the name of the PR that this will convert VIC 4.2 ASCII state files into VIC 5.0 netcdf state files. It's not generic at this time (as @tbohn already points out).

…vis.html

Fixed miniconda setup in .travis.yml.

tbohn · 2017-07-03T23:24:41Z

I squashed Test 1-5 of .travis.yml and then did a git push --force origin.

tbohn · 2017-07-12T19:46:44Z

Just a note - since issue #35 was modified last year to only pertain to the ascii -> netcdf direction (the opposite direction was split out into issue #44 ), accepting this PR will allow us to close issue #35.

dgergel

@tbohn here's my review - I've noted some changes in names that I think you should make, asked for some further clarification/comments and some structural mods, and I think you also have some changes showing up in here that are not part of your PR.

dgergel · 2017-07-12T20:08:35Z

forcing_tools/netcdf2vic.py

@@ -0,0 +1,372 @@
+#!/usr/bin/env python


I'm finding this naming convention to be a bit confusing. I think a more informative name for this would be netcdf2ascii.py.

dgergel · 2017-07-12T20:10:35Z

forcing_tools/netcdf2vic.py

+class Point(object):
+    '''
+    Creates a point class for intellegently storing coordinate information and
+    writing to disk


intellegently --> intelligently

dgergel · 2017-07-12T20:12:48Z

forcing_tools/netcdf2vic.py

+    # Make sure OUTPATH is available
+    if not os.path.exists(config_dict['OPTIONS']['OUTPATH']):
+        os.makedirs(config_dict['OPTIONS']['OUTPATH'])
+    # ---------------------------------------------------------------- #


I think it is better to use the exist_ok=True option here to remove the if statement

dgergel · 2017-07-12T20:14:40Z

forcing_tools/netcdf2vic.py

+        raise ValueError('Binary output is not yet complete, use ascii')
+        for p in points:
+            p._open_binary()
+            p.append = p._append_binary


Confused what is going on here - you're raising a ValueError but still looping over points?

dgergel · 2017-07-12T20:18:57Z

forcing_tools/netcdf2vic.py

+            return int(value)
+        elif isfloat(value):
+            return float(value)
+        else:


I'm a bit confused as to what's going on here - need more comments, but it also seems clunky

dgergel · 2017-07-12T20:41:21Z

tonic/models/vic/grid_params.py

+
+
+# -------------------------------------------------------------------- #
+def state(state_file, soil_dict, veg_dict, lake_dict, version_in='4.2',


I would rename this to read_statefile or something like that

dgergel · 2017-07-12T20:43:38Z

tonic/models/vic/vic.py

+        --------
+        retcode = vic.run(global_param_path, logdir=".", mpi_proc=4)
+
+        """


I might be wrong, but it looks like some of these changes came from a different PR but are showing up as part of yours. the mpi_proc=4 I remember adding to tonic functionality to deal with MPI changes in VIC.

dgergel · 2017-07-12T20:44:23Z

tonic/models/vic/vic.py

@@ -51,7 +84,7 @@ def run(self, global_param, logdir=None):
            with open(global_param_file, mode='w') as f:
                f.write(global_param)

-        self._call_vic('-g', global_param_file)
+        self._call_vic('-g', global_param_file, **kwargs)



Yeah, I don't think this is part of your PR - Joe and I made these changes before.

dgergel · 2017-07-12T20:44:43Z

tonic/models/vic/vic.py

+
+        self.argstring = ' '.join(self.args)
+
+        proc = subprocess.Popen(self.argstring,


Same as above.

dgergel · 2017-07-12T20:45:34Z

tonic/testing.py

-# -------------------------------------------------------------------- #
-def check_forcings_integrity():
-    return
-# -------------------------------------------------------------------- #


Why is this all deleted?

tbohn · 2017-07-12T20:56:25Z

Thanks @dgergel you're right, there appears to be a lot of stuff in here that I didn't do. I don't know how those changes got in - I might have accidentally merged some stuff from feature/vic42 into here. Or something else.

The only changes I made were to grid_params.py and possibly vic_util.

Maybe I should close this PR and redo it cleanly?

dgergel · 2017-07-12T21:00:43Z

@tbohn yes I think it needs to be redone so the PR only has the code changes that you made. There's also a bunch of code re-formatting stuff in here that looks like it does need to be reformatted but should go into a separate PR. While you're doing that, it would also be helpful at the beginning of the PR to have a detailed list of changes that are going into it, e.g. new scripts and what they do. We've been stricter about that lately for VIC development and it's very helpful (e.g. it reminds you to run a format-checker before requesting code reviews)

tbohn · 2017-07-12T21:02:42Z

What do you mean by format checker - uncrustify? And these format problems, are they in grid_params.py or the other stuff that's not supposed to be in my PR?

dgergel · 2017-07-12T21:04:55Z

Yep, uncrustify. I meant that there are formatting changes in the PR that look like they need to be made (e.g. lines are too long) but these changes should be made in a different PR because they aren't specific to what you did. We just probably haven't run uncrustify on tonic for a while is my guess.

tbohn · 2017-07-13T02:58:38Z

OK, I'm closing this and will open a new PR shortly.

Joe Hamman and others added 18 commits April 2, 2014 10:42

add ascii header option to vic2netcdf.py -- ignores standard vic 6 li…

737ed0f

…ne header

Merge branch 'master' of github.com:jhamman/VICpy into feature/vic42

1371ff0

major upgrade to netcdf2vic.py, fixes memory and speed issues experie…

7e2a902

…nced with large datasets, binary output files are not supported in this version

Merge pull request UW-Hydro#21 from UW-Hydro/develop

c3c957e

merge develop into master for tonic 0.1

Final mods.

2815e33

Fixed output data type of integer params.

71afa5b

Updates related to review comments.

e094bae

Removed debugging print statement.

97e912b

Merge pull request UW-Hydro#40 from tbohn/feature/add_support_for_lak…

a268595

…e_blosnow_veghist Added support for lake, blowing snow, and flexible lai/fcanopy/albedo to grid_params.py

Initial commit

97e6b32

Final edits.

f01f659

Fixed a couple bugs.

87cca44

Fixed typo in variable name.

31741b3

Fixed another typo.

69189f6

Now reads either C3/C4 or 0/1 for Ctype.

d815d1a

Merge pull request UW-Hydro#41 from tbohn/feature/add_carbon_params

8b1c5c0

Added support for carbon parameters to grid_params.py

Added ability to convert ascii state files to netcdf

4edde68

tbohn mentioned this pull request Mar 21, 2016

Add support for state files #35

Open

Updated variable names to match those in VIC 5.0.

6a81957

dgergel reviewed Mar 22, 2016
View reviewed changes

Joe Hamman added 7 commits May 17, 2016 16:46

cleanup commit (UW-Hydro#55)

7b09158

update grid_params.py

57b2813

update mask type in vic parameters

fe8f6d3

use mpiexec instead of mpirun in vic object (UW-Hydro#57)

bfc6f41

use valgrind supressions file when available via env variable (UW-Hyd…

958fe10

…ro#58)

update vic_valgrind_suppressions.supp filename (UW-Hydro#59)

843d050

fix typo in VIC_VALGRIND_SUPPRESSIONS (UW-Hydro#60)

190aca2

* update vic_valgrind_suppressions.supp filename * fix another typo in VIC_VALGRIND_SUPPRESSIONS

Merge remote-tracking branch 'upstream/feature/vic42' into feature/vic42

4693b6c

Conflicts: forcing_tools/netcdf2vic.py

tbohn added 2 commits June 28, 2017 13:56

Merged changes from feature/vic42 into this branch.

0e8563d

Merge branch 'feature/vic42' into feature/add_statefile

2c4c04e

Conflicts: tonic/models/vic/grid_params.py

jhamman changed the base branch from master to develop June 29, 2017 17:02

Updated .travis.yml to look like example on https://conda.io/docs/tra…

5cd1b42

…vis.html

tbohn changed the title ~~Added ability to convert ascii state files to netcdf~~ Added ability to convert VIC 4.2 ascii state files to VIC 5.0 netcdf Jul 3, 2017

tbohn mentioned this pull request Jul 3, 2017

Feature/add statefile #64

Closed

Added default arg of "None" for state file.

0f80873

Fixed miniconda setup in .travis.yml.

tbohn force-pushed the feature/add_statefile branch from 7db326c to 0f80873 Compare July 3, 2017 23:23

dgergel suggested changes Jul 12, 2017

View reviewed changes

tbohn closed this Jul 13, 2017

tbohn deleted the feature/add_statefile branch July 13, 2017 03:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added ability to convert VIC 4.2 ascii state files to VIC 5.0 netcdf #43

Added ability to convert VIC 4.2 ascii state files to VIC 5.0 netcdf #43

tbohn commented Mar 17, 2016 •

edited

Loading

tbohn commented Mar 19, 2016

tbohn commented Mar 20, 2016

jhamman commented Mar 20, 2016

tbohn commented Mar 20, 2016

tbohn commented Mar 20, 2016

tbohn commented Mar 20, 2016

tbohn commented Mar 20, 2016

dgergel Mar 22, 2016

tbohn Mar 22, 2016

jhamman Mar 23, 2016

tbohn Mar 23, 2016

tbohn commented Jun 13, 2017

jhamman commented Jun 23, 2017

tbohn commented Jun 28, 2017

bartnijssen commented Jun 29, 2017 •

edited

Loading

tbohn commented Jul 3, 2017

tbohn commented Jul 12, 2017

dgergel left a comment

dgergel Jul 12, 2017

dgergel Jul 12, 2017

dgergel Jul 12, 2017

dgergel Jul 12, 2017

dgergel Jul 12, 2017

dgergel Jul 12, 2017

dgergel Jul 12, 2017

dgergel Jul 12, 2017

dgergel Jul 12, 2017

dgergel Jul 12, 2017

tbohn commented Jul 12, 2017

dgergel commented Jul 12, 2017

tbohn commented Jul 12, 2017

dgergel commented Jul 12, 2017

tbohn commented Jul 13, 2017



		# -------------------------------------------------------------------- #
		def state(state_file, soil_dict, veg_dict, lake_dict, version_in='4.2',


		self.argstring = ' '.join(self.args)

		proc = subprocess.Popen(self.argstring,

Added ability to convert VIC 4.2 ascii state files to VIC 5.0 netcdf #43

Added ability to convert VIC 4.2 ascii state files to VIC 5.0 netcdf #43

Conversation

tbohn commented Mar 17, 2016 • edited Loading

tbohn commented Mar 19, 2016

tbohn commented Mar 20, 2016

jhamman commented Mar 20, 2016

tbohn commented Mar 20, 2016

tbohn commented Mar 20, 2016

tbohn commented Mar 20, 2016

tbohn commented Mar 20, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbohn commented Jun 13, 2017

jhamman commented Jun 23, 2017

tbohn commented Jun 28, 2017

bartnijssen commented Jun 29, 2017 • edited Loading

tbohn commented Jul 3, 2017

tbohn commented Jul 12, 2017

dgergel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbohn commented Jul 12, 2017

dgergel commented Jul 12, 2017

tbohn commented Jul 12, 2017

dgergel commented Jul 12, 2017

tbohn commented Jul 13, 2017

tbohn commented Mar 17, 2016 •

edited

Loading

bartnijssen commented Jun 29, 2017 •

edited

Loading