-
Notifications
You must be signed in to change notification settings - Fork 414
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
Fix run_cell bug in image driver #662
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. |
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.
Looks good but I have a few small questions.
} | ||
|
||
// get lat and lon coordinates | ||
get_nc_latlon(domain_nc_name, global_domain, global_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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be interested to hear what @bartnijssen thinks is best here.
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bartnijssen It shows - and your suggestion sounds good. Will pin you when it's done.
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.
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
mask
variable in the domain file in image driver. Now after the fix in this PR,run_cell
variable in the parameter file controls active cells. Specifically, were-organized
get_global_domain
function to:1) use
run_cell
variable from the parameter file to determineactive 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.
Some docs updates related to this change is also incorporated. The Stehekin sample dataset is updated to be unambiguous in the
VIC_sample_data
repo (UW-Hydro/VIC_sample_data#16).