This repository has been archived by the owner on Jun 30, 2023. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 4
Edit search radius used for site-to-reach matching #116
Merged
Merged
Changes from all commits
Commits
Show all changes
2 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Why *2 now? Is this to allow NWIS sites to be matched and later retained? If that's why, I think it would be better to add a separate search radius column for the NWIS sites to retain. This would allow for still retaining NWIS sites when the
search_radius
is reduced to something smaller than would match the NWIS sitesThere 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.
Yeah, good question. I made this change to initially use a wider search radius when we apply
nhdplusTools::get_flowline_index()
. That function uses a radius search implemented inRANN::nn2()
and I noticed when looking at DO data that a couple sites were not being picked up with a 500 m search radius (but were picked up with a 1000 m radius) even though the distance between those sites was < 500 m, as confirmed by the offset returned and separately using {sf}. More details are here.So this doesn't have anything to do with the two NWIS sites but rather, to ensure we're picking up all the sites we would expect initially, which we then filter to the requested
search_radius
a couple lines down. I'm open to other suggestions, too, so happy to hear your thoughts!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.
Strange that those 2 sites do not get picked up! I'm not familiar with the
nn2
function, and the source is in C, so I'm less inclined to look into if it's a matter of using a different argument inget_flowline_index
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.
Yeah, I wasn't worried about those two sites in particular, but the behavior was puzzling to me. The distances returned seem to be correct, so for now I decided to just adjust the argument in
get_flowline_index
to make sure we're capturing all the sites we'd expect. I tried to explain this reasoning in the comments because I know the different argument to search_radius is a little odd.