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

specify netcdf variable names in global parameter file #379

Merged
merged 1 commit into from Feb 19, 2016

Conversation

Projects
None yet
4 participants
@jhamman
Member

jhamman commented Jan 31, 2016

This PR closes #211. Forcing variables can be specified in the global parameter following the convention proposed in #211.

blocked by #378

@jhamman jhamman added this to the 5.0 milestone Jan 31, 2016

@jhamman jhamman self-assigned this Jan 31, 2016

@jhamman

This comment has been minimized.

Show comment
Hide comment
@jhamman

jhamman Feb 1, 2016

Member

This is ready for a review from @bartnijssen or @tbohn, although it may be easier to wait until after #378 is merged.

Member

jhamman commented Feb 1, 2016

This is ready for a review from @bartnijssen or @tbohn, although it may be easier to wait until after #378 is merged.

@jhamman

This comment has been minimized.

Show comment
Hide comment
@jhamman

jhamman Feb 1, 2016

Member

@wietsefranssen - I reworked your COORD_DIMS_OUT changes in the last commit. Can you take a look and tell me what you think?

Member

jhamman commented Feb 1, 2016

@wietsefranssen - I reworked your COORD_DIMS_OUT changes in the last commit. Can you take a look and tell me what you think?

@@ -41,7 +41,6 @@ double calc_netshort(double, int, double, double *);
void check_files(filep_struct *, filenames_struct *);
FILE *check_state_file(char *, size_t, size_t, int *);
void close_files(filep_struct *, out_data_file_struct *, filenames_struct *);
size_t count_force_vars(FILE *gp);

This comment has been minimized.

@bartnijssen

bartnijssen Feb 2, 2016

Member

Why is this not needed for classic? Wasn't this just introduced? Does that mean that in vic_classic the user still needs to specify the number of forcing variables?

@bartnijssen

bartnijssen Feb 2, 2016

Member

Why is this not needed for classic? Wasn't this just introduced? Does that mean that in vic_classic the user still needs to specify the number of forcing variables?

This comment has been minimized.

@jhamman

jhamman Feb 2, 2016

Member

This was just moved to driver/shared

@jhamman

jhamman Feb 2, 2016

Member

This was just moved to driver/shared

}
// define the netcdf variable longitude
status = nc_def_var(nc->nc_id, options.DOMAIN_LON_VAR, NC_DOUBLE, ndims,
status = nc_def_var(nc->nc_id, global_domain.info.lon_var, NC_DOUBLE, ndims,

This comment has been minimized.

@bartnijssen

bartnijssen Feb 2, 2016

Member

In the code shouldn't this just be XDIM and YDIM, which may be lat and lon, but could be something different (equal area grids, different projections, etc).

@bartnijssen

bartnijssen Feb 2, 2016

Member

In the code shouldn't this just be XDIM and YDIM, which may be lat and lon, but could be something different (equal area grids, different projections, etc).

This comment has been minimized.

@jhamman

jhamman Feb 2, 2016

Member

This uses the coordinate names from the domain file. This will handle regular or irregular grids without any issues.

@jhamman

jhamman Feb 2, 2016

Member

This uses the coordinate names from the domain file. This will handle regular or irregular grids without any issues.

@@ -1250,6 +1250,7 @@
_Bool SIGNED;
_Bool SUPPLIED;
double multiplier;
char varname[2048];

This comment has been minimized.

@bartnijssen

bartnijssen Feb 2, 2016

Member

Don't use a hard coded literal. Abstract this as MAXSTRING or something.

@bartnijssen

bartnijssen Feb 2, 2016

Member

Don't use a hard coded literal. Abstract this as MAXSTRING or something.

This comment has been minimized.

@jhamman

jhamman Feb 2, 2016

Member

The python headers are "preprocessed". So all the variables are expanded. At some point, we'd like to create these at build time on the fly but that is a someday task at this point.

@jhamman

jhamman Feb 2, 2016

Member

The python headers are "preprocessed". So all the variables are expanded. At some point, we'd like to create these at build time on the fly but that is a someday task at this point.

@bartnijssen

This comment has been minimized.

Show comment
Hide comment
@bartnijssen

bartnijssen Feb 2, 2016

Member

May be worth a second look or a bit of discussion about the comments above.

Member

bartnijssen commented Feb 2, 2016

May be worth a second look or a bit of discussion about the comments above.

@jhamman

This comment has been minimized.

Show comment
Hide comment
@jhamman

jhamman Feb 17, 2016

Member

I've address all of @bartnijssen's original comments. As I mentioned before, changing the names of SHORTWAVE and LONGWAVE increased the scope of this PR but I think it was a nice set of improvements.

@wietsefranssen - I'd still appreciate you looking over this if you can get a chance.

This needs a rebase/squash then it should be ready to merge (pending no further comments).

Member

jhamman commented Feb 17, 2016

I've address all of @bartnijssen's original comments. As I mentioned before, changing the names of SHORTWAVE and LONGWAVE increased the scope of this PR but I think it was a nice set of improvements.

@wietsefranssen - I'd still appreciate you looking over this if you can get a chance.

This needs a rebase/squash then it should be ready to merge (pending no further comments).

@wietsefranssen

This comment has been minimized.

Show comment
Hide comment
@wietsefranssen

wietsefranssen Feb 19, 2016

Collaborator

I've looked at all @bartnijssen's comments and I agree with all of them. I also agree in changing the radiations names (and on to choose from different options). This avoids confusion.

good work!

Collaborator

wietsefranssen commented Feb 19, 2016

I've looked at all @bartnijssen's comments and I agree with all of them. I also agree in changing the radiations names (and on to choose from different options). This avoids confusion.

good work!

jhamman added a commit that referenced this pull request Feb 19, 2016

Merge pull request #379 from jhamman/feature/force_ncvars
specify netcdf variable names in global parameter file

@jhamman jhamman merged commit 8340250 into UW-Hydro:develop Feb 19, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@jhamman jhamman deleted the jhamman:feature/force_ncvars branch Feb 19, 2016

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