Skip to content
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

Accept single polygons as prediction area boundaries #146

Merged
merged 4 commits into from
Jun 30, 2023
Merged

Conversation

mikemahoney218
Copy link
Member

This PR fixes #145 by adjusting the behavior of spatial_nndm_cv(). If prediction_sites is of length 1 and a polygon or multipolygon, then the function will now sample within that boundary instead of within its bounding box.

@mikemahoney218
Copy link
Member Author

Howdy @hfrick -- this PR adds a feature to spatial_nndm_cv() that I personally wanted for a project of mine 😄 I'm pretty confident on the spatial stuff here, but would appreciate any comments on style you might have!

Copy link
Member

@hfrick hfrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice one! Just a minor comment about the ordering of things ☀️

Comment on lines 121 to 145

# these are more for clarity than control flow -- they do not get used
# outside of the below if/else
use_provided_points <- length(pred_geometry) == 1 && pred_geometry == "POINT"
sample_provided_poly <- length(pred_geometry) == 1 && pred_geometry %in% c(
"POLYGON",
"MULTIPOLYGON"
)
sample_bbox <- !use_provided_points && !sample_provided_poly

if (sample_bbox) {
sample_points <- sf::st_sample(
x = sf::st_as_sfc(sf::st_bbox(prediction_sites)),
size = prediction_sample_size,
...
)
} else if (sample_provided_poly) {
sample_points <- sf::st_sample(
x = sf::st_geometry(prediction_sites),
size = prediction_sample_size,
...
)
} else if (use_provided_points) {
sample_points <- prediction_sites
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it!

Imo you can drop the comment about this being about clarity, I'd say that's a well-established pattern and doesn't need the explanation.

I'd make the order of the ifelse statement the same as you laid it out above: sampling from the bounding box is the fall back so it shouldn't be the first condition to check. If the order is polygon > points > bbox, then that should be the order for both the definitions up in line 124-128 (no need for sample_bbox unless you want it) and the condition checking in the if-else block.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes a ton of sense, thanks!

@mikemahoney218 mikemahoney218 merged commit 0d6cf09 into main Jun 30, 2023
@github-actions
Copy link

This pull request has been automatically locked. If you believe you have found a related problem, please file a new issue (with a reprex: https://reprex.tidyverse.org) and link to this issue.

@github-actions github-actions bot locked and limited conversation to collaborators Jul 15, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
2 participants