-
Notifications
You must be signed in to change notification settings - Fork 393
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
Outfile Dimensions #303
Outfile Dimensions #303
Conversation
…e/outfile_time * 'develop' of https://github.com/UW-Hydro/VIC: fix rounding / precision bug at midnight with subdaily timestep add python 3.5 to travis print traceback when log_err is called
…able in NetCDF output
@@ -63,6 +70,7 @@ typedef struct { | |||
size_t n_nx; /**< size of x-index; */ | |||
size_t n_ny; /**< size of y-index */ | |||
location_struct *locations; /**< locations structs for local domain */ | |||
location_grid_struct *locations_grid; /**< locations structs for full unmasked latlon domain */ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm wondering if there is a better way to do this. Perhaps we could create a location_struct
for all grid cells, even if they are not being run. We would just need to add an active
(or something similar) member to the location_struct
. When we do the decomposition, we would only split up active grid cells so nothing would change below the MASTER_PROC
level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is possible, but keep in mind that in that case the indexes are changing. I mean the following:
In the case you suggest, location_struct
is bigger because the unmasked gridcells are now included. This means that location_struct[n]
refers to another lonlat combination. We have to be very careful that this will not effect other parts of the VIC code.
What shall we do now to solve it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, let's leave this as is here and we'll revisit it in #313.
@wietsefranssen - good start here. I made a bunch of comments to keep this moving. |
global_domain->locations_grid = (location_grid_struct *) | ||
malloc( | ||
global_domain->n_ny * global_domain->n_nx * | ||
sizeof(location_grid_struct)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see #257 and update the cast syntax.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed it into (made also a calloc of it):
global_domain->locations_grid = calloc( global_domain->n_ny * global_domain->n_nx, sizeof(*global_domain->locations_grid));
I just used the same syntax as a few lines earlier eg line 76. Why do these one still use the old cast syntax? Or just because you didn't push the last commit to github yet?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That PR is still in the works (see #257).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I'll wait for a next commit :)
…ts (cast syntax).
@wietsefranssen : Can you resolve the merge conflicts before I merge this? Since you started this branch a while ago, you probably need to merge the existing develop into your branch first. After that you probably won't have any merge conflicts. |
@bartnijssen - I don't think we're ready to merge here yet. We still need to resolve the calendar and time handling pieces. |
OK - sounds good - still need to resolve the conflict though. |
derived time units from 'global_param' to output NetCDF. fixed time init by adding 'initialize_time()' to 'vic_init.c'
sprintf(strUnit, "days"); | ||
} | ||
else { | ||
sprintf(strUnit, "-"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better to raise an error here if we don't know what the time units are.
|
||
grid_size = global_domain.n_ny * global_domain.n_nx; | ||
|
||
// allocate memory for variables to be stored | ||
dvar = (double *) malloc(local_domain.ncells * sizeof(double)); | ||
dvar = calloc(local_domain.ncells, sizeof(*dvar)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
did you mean to change this from malloc
to calloc
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but it was a mistake of me while adapting the cast syntax you mentioned. I am used to use calloc in stead of malloc (calloc is a bit slower because it it zeroing out memory is'nt it?). anyway I changed it to:
dvar = malloc(local_domain.ncells * sizeof(*dvar));
Also some minor adjustments
initialize_location_grid(location_grid_struct *location_grid) | ||
{ | ||
location_grid->latitude = 0; | ||
location_grid->longitude = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you use MISSING
instead of 0 here?
@wietsefranssen - almost there. I'm working on #312 today. |
@jhamman Hi Joe, I just made some changes in order to retreive the number of dimensions from the domain file. I also removed the |
*****************************************************************************/ | ||
int | ||
get_nc_varndimensions(char *nc_name, | ||
char *var_name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the indentation looks off here.
This looks pretty good. I had a few minor comments. Once you've addressed those, please ping @bartnijssen and @tbohn for their code reviews. |
… number or coordinate dims. Also a minor bug fix in vic_write.c
@jhamman I just fixed the things Joe suggested. @tbohn and @bartnijssen Can you please have a look at the code? Thank you beforehand. |
@tbohn and @bartnijssen - last call for comments here. @wietsefranssen - can you merge in the current I'll give this one last read through and then merge if there are no failing tests and no objections by others. |
this pr closes #297 |
…e/outfile_time
Merging now. Thanks @wietsefranssen. |
We have added lat, lon and time variables in the NetCDF output files. One can choose between 1D and 2D lat/lon variables by setting COORD_DIMS_OUT in the global parameter file.
Adapted time dimension in NetCDF output:
Number of timesteps was based on MODEL_STEPS_PER_DAY in the global parameter file.
Now the number of timesteps is based on OUTPUT_STEPS_PER_DAY in the global parameter file.