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

tbohn · 2017-07-13T03:41:15Z

The state file feature required modifying 2 files: tonic/scripts/vic_utils (to add statefile-related arguments) and tonic/models/vic/grid_params.py.

Also modified .travis.yml to get miniconda working properly.

closes issue Add support for state files #35

… Also modified .travis.yml to get miniconda working properly.

dgergel

This looks mostly pretty good to me, but I have a few suggestions for changes.

dgergel · 2017-08-01T22:29:54Z

tonic/models/vic/grid_params.py

+
+        self.soil_param['off_gmt'] = np.array([i])
+        i += 1
+


I'm not a fan of repeating the i += 1 line over and over - this seems pretty clunky to me. And is i actually always the value that you want, because it doesn't seem to be resetting...

OK, I'm making this more dynamic, although it might still be somewhat clunky. Can we improve the code on a separate PR?

dgergel · 2017-08-01T23:02:53Z

tonic/models/vic/grid_params.py

+                k = mydict[var].shape[2]
+                l = mydict[var].shape[3]
+                out_dict[var] = np.ma.zeros((j, k, l, ysize, xsize),
+                                            dtype=dtype)


Can't you just return a tuple and put this into the np.ma.zeros line, e.g. out_dict[var] = np.ma.zeros((mydict[var].shape), ysize, xsize? I guess you might need to convert the tuple to an array but I think it'd be cleaner to combine these two steps...

The i, j, and k are used a couple lines later in the assignment of out_dict (e.g., line 1227: for jj in pyrange(j): etc., then line 1230: out_dict[var][jj, kk, ll, yi, xi] = mydict[var][:, jj, kk, ll]). Whoever had worked on grid_params.py as of spring 2016 (jhamman?) had implemented that logic. I assume the purpose was to map the mydict values, which only contain cells within the domain/basin, to out_dict, which additionally contains fill_values. I simply extended the logic to handle those state variables that have different dimensions than the variables found in the soil_param file. Can you suggest a better way of doing the assignment?

Personally I think that overhauling someone else's inefficient code should be part of a different PR. The purpose of my PR was simply to add needed functionality. If the code prior to this PR was OK to merge into the archive, then my PR should be held to the same standard.

Fair enough. Works for me.

Thanks. Note: when I say "someone else's inefficient code" I don't in any way mean to imply that I'm a great python coder. I'm just saying that I hadn't budgeted for doing an overhaul of the code, and hadn't known at the time (last year) that there was anything in the existing code that needed overhauling.

No I totally agree. I think it's way better for version control to separate out PRs. Since this is a new feature it should be a standalone PR without any other overhauls.

So I'm completely in agreement with you that that should be a separate thing.

dgergel · 2017-08-01T23:06:06Z

tonic/models/vic/grid_params.py

+                l = mydict[var].shape[3]
+                m = mydict[var].shape[4]
+                out_dict[var] = np.ma.zeros((j, k, l, m, ysize, xsize),
+                                            dtype=dtype)


Same comment as above

Same reply as above.

dgergel · 2017-08-01T23:07:24Z

tonic/models/vic/grid_params.py

+        else:
+            new[:, :, :] = out_dicts['veg_dict'][var]
+        new[lib_bare_idx, :, :] += bare
+        # Ensure that Cvs sum to 1.0
        new /= new.sum(axis=0)


Throw a warning if they don't?

dgergel · 2017-08-01T23:10:07Z

tonic/models/vic/grid_params.py

+            shape = (nveg_classes, veglib_dict[lib_var].shape[1],
+                     ysize, xsize)
+            new = np.full(shape, FILLVALUE_F)
+            if extra_class:


shouldn't this be fill_val instead of the float or int specific ones?

since fill_val will have already been assigned to the integer or float fillvalue

This looks good to me.

dgergel

@tbohn these changes look good to me.

dgergel · 2017-08-08T18:29:04Z

tonic/models/vic/grid_params.py

+            shape = (nveg_classes, veglib_dict[lib_var].shape[1],
+                     ysize, xsize)
+            new = np.full(shape, FILLVALUE_F)
+            if extra_class:


This looks good to me.

tbohn · 2017-08-08T18:33:00Z

Thanks @dgergel.

jhamman

just some minor syntax comments. looks good though.

jhamman · 2017-08-22T18:44:34Z

scripts/vic_utils

+                                    help="Flag indicating whether state "
+                                         "file contains carbon-cycle "
+                                         "variables (default=False)",
+                                    default=False)


for the options that are just boolean flags, lets do:

type=bool, default=False, action='store_true', help='...')

Ah, ok, that makes sense. I see that all of the code I've added in this function suffers from this problem. So I'll fix all instances of it, including instances from previous PRs. Not sure why I did it that way, perhaps initially I didn't know about the bool type; but along the way I did learn about the bool type yet I just mindlessly copied the existing code.

I see some instances in ncparam2ascii_parser in which the default value of a str or int option is also False. I won't fix those, but I'm calling your attention to them in case you want to open an issue about it.

Note as well that you are proposing a new option order (I realize this is a purely cosmetic issue). I've reordered as you requested for the boolean options. The other options are unchanged. Again, I propose that if you want all calls to add_argument() to have a consistent order, it should be a new issue/PR. But calling it to your attention.

Well, I had to roll back the changes that I made in response to your suggestion - they broke the travis build. I've kept the change in type to bool. But I've not added the action=store_true or the move of default=False to be second in the order.

What was the error you were getting? This should work.

It had a problem with type not recognized or something like that. I don't have the error accessible anymore. The error message doesn't make sense of course, since type is still there in the same position in the arg list.

Can you look into that issue separately, and deal with it in some other PR? It's not related to the change I'm making.

Actually, I found my travis build log. The error was the same in python 2.7 and 3.4:
TypeError: init() got an unexpected keyword argument 'type'

Ok. Take a look at the docs. Looks like you just need the action='store_true'. No default/dtype is required.

https://docs.python.org/3/library/argparse.html#action

OK, I replaced the type=bool and default=False arguments with action='store_true'.

jhamman · 2017-08-22T18:46:38Z

tonic/models/vic/grid_params.py

+            self.veg_param['sigma_slope'] = 'Std. deviation of terrain ' \
+                                            + 'slope within veg tile'
+            self.veg_param['lag_one'] = 'Lag one gradient autocorrelation ' \
+                                        + 'of terrain slope'


the backslash and plus sign are not needed here. Python can do the string line continuation as:

a = 'start of the ' \ 'line'

apply this fix throughout your code above/below.

When I attempt that, my syntastic add-on to vim complains about an unexpected indent. But I'll make the change as you suggest regardless...

Not only does my editor complain, but when I attempt to run tonic in my python environment, python complains about unexpected indent on the continued lines. Restoring the \ and + appeases my python. I'm running Python 3.5.

@tbohn - My apologies, my comment was not entirely correct. You don't need the + or \ if you are inside parentheses. When you are not (as in this case), you need the backslash but not the plus sign.

ref: https://stackoverflow.com/questions/5437619/python-style-line-continuation-with-strings

jhamman · 2017-08-22T18:51:35Z

tonic/models/vic/grid_params.py

+            new = np.full(shape, fill_val)
+            if extra_class:
+                new[:-1, :, :] = out_dicts['veg_dict'][var]
+                new[-1, :, :] = bare_vegparam[var]


trailing colons are not used in numpy. I see they were there before but they are not needed so lets remove them. While we're at it, let's rework this section to look like:

if extra_class: shape = (nveg_classes, ) + out_dicts['veg_dict'][var].shape[1:] new = np.full(shape, fill_val) new[:-1] = out_dicts['veg_dict'][var] new[-1] = bare_vegparam[var] else: new = out_dicts['veg_dict'][var]

You don't have to fix this colon syntax everywhere, just in lines you've changed :)

Hmm, I'm wondering if the line shape = (nveg_classes, ) + out_dicts['veg_dict'][var].shape[1:] is correct... I need to properly test that case.

But sure, I'll get rid of the trailing colons.

Question: what exactly do you mean about trailing colons not being used in numpy? I've used them a lot and everything has worked fine. Do you mean that trailing colons are not necessary in numpy, and that you'd like to adopt a convention in which they're not used?

jhamman · 2017-08-22T18:53:25Z

tonic/models/vic/grid_params.py

+                dtype_str = 'np.int'
+            else:
+                fill_val = FILLVALUE_F
+                dtype_str = 'np.float'


do these dtypes need to be cast to strings? I don't think they do...

Ah, OK, I guess they are like pre-compiler constants in C/Fortran, rather than strings. Makes sense.

…guments statements.

…bool and default=False.

tbohn · 2017-08-28T18:06:43Z

Please wait to merge this PR; I'm debugging something on a different branch that might impact this branch. Stay tuned...

tbohn · 2017-08-28T20:52:11Z

OK, this is ready for merge now.

tbohn added 2 commits July 12, 2017 20:30

Added ability to convert VIC 4.2 ascii state files to VIC 5.0 netcdf.…

a9b8812

… Also modified .travis.yml to get miniconda working properly.

Reformatted to make vim/syntastic happy.

609d03c

dgergel suggested changes Aug 1, 2017

View reviewed changes

tbohn added 9 commits August 5, 2017 15:05

Updates to addressing pull request comments.

4af7e58

Removed extraneous file.

e74712a

Fix small typo.

fdc8543

Test

f48e7dd

Test

5d19c03

Test

7a380dc

Test

583f5a7

Test

3acd668

Fixed unhashable list error.

1f6d73d

dgergel approved these changes Aug 8, 2017

View reviewed changes

jhamman reviewed Aug 22, 2017

View reviewed changes

tbohn added 3 commits August 27, 2017 02:37

Misc updates in response to review comments.

f67d3fc

Rolled back the change in the order of arguments in the parser.add_ar…

59c4f52

…guments statements.

Added the action='store_true' arguments back in, and eliminated type=…

7549df5

…bool and default=False.

Made depth a 3-column variable.

1bc1bc3

tbohn added 2 commits August 28, 2017 13:56

Removed unnecessary '+' line continuation symbols.

8d1cef4

Removed more unnecessary '+' line continuation symbols.

42e40fc

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

Are you sure you want to change the base?

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

Conversation

tbohn commented Jul 13, 2017 • edited Loading

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

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

dgergel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbohn commented Aug 8, 2017

jhamman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tbohn Aug 22, 2017 • edited Loading

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

jhamman Aug 22, 2017 • edited Loading

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 Aug 28, 2017

tbohn commented Aug 28, 2017

tbohn commented Jul 13, 2017 •

edited

Loading

tbohn Aug 22, 2017 •

edited

Loading

jhamman Aug 22, 2017 •

edited

Loading