Fix run_cell bug in image driver #662
Conversation
re-organized `get_global_domain` function to:
1) use `run_cell`variable from the parameter file to determine
active cell (instead of the `mask` variable in the domain file);
2) lat and lon coordinates loading is put into a separated function;
3) the step of checking whether the latlons in the parameter file
match the domain file is moved to early on in the `get_global_domain`
function.
|
@jhamman this PR is ready for review. |
| } | ||
|
|
||
| // get lat and lon coordinates | ||
| get_nc_latlon(domain_nc_name, global_domain, global_domain); |
There was a problem hiding this comment.
This is an odd call with ...global_domain, global_domain
| void | ||
| get_nc_latlon(char *nc_name, | ||
| domain_struct *global_domain, | ||
| domain_struct *nc_domain) |
There was a problem hiding this comment.
can you help me understand how these two domain structs differ and what you intend for nc_domain to be. Perhaps some more documentation in the header is appropriate.
There was a problem hiding this comment.
Hmm, this function is to get lat lon information from an nc file. The loaded lat lon info will be stored in nc_domain. Here global_domain is not modified by the function, but only to provide some global information (lat and lon variable names, and total number of grid cells). Perhaps we could directly pass in those global information instead of passing in the whole global_domain structure? (And this is related to your first comment I guess)
There was a problem hiding this comment.
I'd be interested to hear what @bartnijssen thinks is best here.
There was a problem hiding this comment.
I find this confusing as well. It makes the function do more than it needs to anyway. If you are sure that the domain information from global_domain can simply be copied to nc_domain, then do so explicitly before you enter the function. For example, write a small copy_domain(domain_struct *domain_from, domain_struct *domain_to) function. Call that first and then call get_nc_latlon(char *nc_name, domain_struct *nc_domain). That way there is no hidden beahvior (the copy that happens as a side effect in get_nc_latlon().
There was a problem hiding this comment.
Not sure why my comment : https://github.com/UW-Hydro/VIC/pull/662/files#r95245324 does not show here, but it should
There was a problem hiding this comment.
@bartnijssen It shows - and your suggestion sounds good. Will pin you when it's done.
There was a problem hiding this comment.
Weird - it did not show up in the "cover page" earlier .... I blame github
|
@yixinmao : Please address https://github.com/UW-Hydro/VIC/pull/662/files#r95245324 and I'll merge after that |
info into a separate function
|
@bartnijssen I've made the change as you suggested. Should be able to merge. |
Before this PR, active cell is controlled by
maskvariable in the domain file in image driver. Now after the fix in this PR,run_cellvariable in the parameter file controls active cells. Specifically, were-organized
get_global_domainfunction to:1) use
run_cellvariable from the parameter file to determineactive cell (instead of the
maskvariable in the domain file);2) lat and lon coordinates loading is put into a separated function;
3) the step of checking whether the latlons in the parameter file
match the domain file is moved to early on in the
get_global_domainfunction.
Some docs updates related to this change is also incorporated. The Stehekin sample dataset is updated to be unambiguous in the
VIC_sample_datarepo (UW-Hydro/VIC_sample_data#16).