Add target_projection and target_pixelsize to ModelSpec and integrate into AWY, UMH - #2545
Conversation
…ulate it and validation function in spatial inputs natcap#2267
…l or loading datastack natcap#2267
…s of target projection and get projection of spatial dataset natcap#2267
…t projection string or a filepath with that target projection natcap#2267
…emove validation that default projection must be projected; add functionality to select default pixel size based on dropdown_function; check units when checking projection natcap#2267
…updates' into task/2267-update-projection-handling-for-dh
…; refresh pixelsize options (but not selection) when target projection refreshes; existing selections remain selected as long as still valid but invalid selections fallback to first opt in dropdown; return empty result to get tests passing natcap#2267
…points to input that is empty natcap#2267
…points to now-empty AOI for UMH natcap#2267
… to instead check if its empty natcap#2267
…wn inputs that haven’t been entered; allow smart title casing to handle slashes correctly natcap#2267
…patial dropdowns with placeholder text if currently selected (non-default) value is invalid natcap#2267
…pdate-projection-handling-for-dh
davemfish
left a comment
davemfish
left a comment
There was a problem hiding this comment.
@claire-simpson I reviewed most of the Python side of this PR and thought that was a good stopping point for now. I didn't look at the python tests yet, or the Workbench side too closely.
I think the design here is really good overall. I had lots of comments about trying to simplify things here and there, and make things a little easier to read and reason about. But this stuff is necessarily somewhat complicated. Thanks for all your hard work on this!
|
|
||
| # reproject watersheds_path to target_projection | ||
| target_spatial_prj = utils.get_raster_or_vector_projection( | ||
| args[args['target_projection']]) |
There was a problem hiding this comment.
Why is there this nested args[args[...]]? The docstring from execute suggests this should be args['target_projection'].
There was a problem hiding this comment.
Yes, you're right and the docstring is incorrect. args['target_projection'] would just be the input id (e.g., lulc_path) instead of the actual filepath we want, which is retrieved via args['lulc_path']. In addition to correcting the docstring, would a comment help make it clear this nesting is intentional?
| 'fraction', 'cost', 'height', 'kw_price' | ||
|
|
||
| args['target_projection'] (string): (optional) if a non-empty string, | ||
| path to input that defines the target projection and alignment |
There was a problem hiding this comment.
Would it be more accurate to say "path to a GDAL raster/vector" rather than "path to input"? That makes it clear that the value is a filepath rather than a projection string of some kind. Maybe naming the key "target_projection_source" would also help make that clear.
I don't remember we ever discuss the possibility of having a wkt string as the value for this arg either. I know we want Workbench users to be able to select a dataset, but python users might like the flexibility of passing a WKT string or similar. That's a larger decision we can discuss though, if we haven't already.
These same suggestions/questions apply to target_pixelsize
There was a problem hiding this comment.
Yep, this docstring is inaccurate. Would it be helpful to rename this arg target_projection_id?
| f'"{", ".join(str(x) for x in sorted(missing_ws_ids))}"') | ||
|
|
||
| # reproject watersheds_path to target_projection | ||
| target_spatial_prj = utils.get_raster_or_vector_projection( |
There was a problem hiding this comment.
If this is a WKT string, it could be nice to name the variable target_projection_wkt or something like that, just because there are so many different objects that could represent a projection. It's nice to quickly see what this one represents.
| if pygeoprocessing.get_gis_type( | ||
| args[args['target_projection']]) == pygeoprocessing.RASTER_TYPE: | ||
| raster_align_index = base_raster_path_list.index( | ||
| args[args['target_projection']]) |
There was a problem hiding this comment.
Okay, related to my earlier question, this looks like a clear reason why we want target_projection to be a data source rather than just a projection string 👍 I guess the alternative would require this "align_index_source" to be yet another UI input.
| # fallback to aligning everything to the arg with default pixel size | ||
| raster_align_index = base_raster_path_list.index( | ||
| args[args['target_pixelsize']]) | ||
| target_pixel_size = utils.get_raster_pixel_size_in_tgt_projection_units( |
There was a problem hiding this comment.
I would slightly prefer get_raster_pixel_size_in_target_proj_units or get_pixel_size_in_target_proj_units. Don't mean to be too nitpicky, but I figure now is the time to do so. I appreciate the name telling me what units will apply to these values though!
There was a problem hiding this comment.
I would definitely rather nitpick now rather than later when the function potentially more widely used!
| return values | ||
|
|
||
| def preprocess_spatial_reference_args(self, args): | ||
| """Set target_projection and target_pixelsize if they aren't in args |
There was a problem hiding this comment.
We want people to be able to run the model without these new args, right? The Workbench will always make sure they are included, but python users may not include them, and that's okay. This preprocessor will set the default values in that case. This is called from within MODEL_SPEC.setup which is called in the body of every model's execute function. (I'm just reminding myself how this all works).
| ) | ||
| ] | ||
| if default_projection_inputs: | ||
| args['target_projection'] = default_projection_inputs[0].id |
There was a problem hiding this comment.
Doesn't the value of args['target_projection'] need to be the filepath to a spatial dataset? This looks like it would be the id of the Input spec instance instead.
| all_options = self.get_input( | ||
| 'target_pixelsize').dropdown_function(args, self) | ||
| if all_options: | ||
| args['target_pixelsize'] = all_options[0].key |
There was a problem hiding this comment.
If this "target_pixelsize" input has no value, do we always default to the same one as args['target_projection']? Here it looks like we're just choosing whichever is first in the list.
There was a problem hiding this comment.
If this "target_pixelsize" input has no value, do we always default to the same one as args['target_projection']?
Yes, the default behavior is to fallback to the same default target projection value (which as you pointed out above, should actually only occur if the target projection value is a raster). In UMH however, there are different default target_pixelsize values specified per model (and defined in a custom dropdown_function: _get_pixelsize_umh).
Here it looks like we're just choosing whichever is first in the list.
Yes, we are choosing whichever the first value is. In _get_pixel_size_options we are ordering the options such that if a default value exists, that will be the first option in the list of options. If no default value exists (which would occur if the default projection is a vector and no custom dropdown_function is specified), this will just be whatever value is first. The other option that I considered was, in absence of a default value, setting a placeholder "select an option", but this seems contradictory to what we've discussed, i.e., avoiding forcing a user to engage with these target projection and pixelsize dropdowns.
What do you think? Should I add a is_default_pixelsize attribute similar to is_default_projection, or keep the current design where is_default_projection also typically defines the default pixel size, and if a different default pixel size is required for a model, that value gets set in a customized version of _get_pixel_size_option? For the majority of models, the same input can serve as the default projection and pixel size, however this is certainly not universally true, and I recognize that the current design lacks clarity.
There was a problem hiding this comment.
After thinking about this more, I am leaning towards adding a is_default_pixelsize attribute and still supporting the conditional defaults à la UMH. I do think that for the 2 spatial dropdowns we should decide on whether to (a) raise an error if the default value does not pass validation (e.g., because its not projected) as demonstrated for target_projection, or (b) grab the first option in the options list regardless of if its the default. I wrote a comment about this too:
#TODO: or do we want to set [target_projection] just like with target pixel size?
# pros: will find a suitable spatial input if one exists
# cons: less transparent to user?
| args (dict): argument dictionary mapping input keys to input values | ||
|
|
||
| Returns: | ||
| dictionary mapping input keys to preprocessed input values |
There was a problem hiding this comment.
I feel like this function should probably make a copy of the args dict first rather than modifying the one passed by the caller. In general it could be surprising to the caller if the object they passed in was modified.
| try: | ||
| warning_msg = parameter_spec.validate(args[key]) | ||
| if not warning_msg: | ||
| warning_msg = parameter_spec.validate_with_context( |
There was a problem hiding this comment.
Okay, now I understand the need for validate_with_context method on all Inputs!
…e responsive_to to OptionStringInput, target_projection is projected by default, preprocess_spatial_reference_args makes a copy of args natcap#2267
2 participants
Description
Initial PR to add
target_projectionandtarget_pixelsizedropdown inputs. First step is testing adding these to AWY and UMH. AWY will also have an optional new AOI input.There are still some outstanding decisions, namely:
target_projectionif the default input is not projected (as we're now moving away from requiring most inputs to be projected)Advanced Optionstoggle, or something similar, to reduce their visibiltyThis PR also addresses some issues in the workbench where we are directly modifying state, and now that the dropdown menus need to be responsive to more changes (e.g., via
responsive_toand havingtarget_pixelsizedropdown change in response totarget_projectionchanges), there were more opportunities for things to get out of sync and show stale states or run extra unneeded validation.First step to address #2267
Checklist