Fix error in setting max half-width in node search#6547
Merged
PaulWessel merged 3 commits intomasterfrom Apr 10, 2022
Merged
Conversation
When we need to access all nodes inside a search radius, we first determine limits on the number of rows and columns away from the center. For geographic data this also means we consider the change in longitudinal distance with latitude. All this works fine. However, we had a "safety valve" that set the upper limit of how many columns we may want to search to either side of a node to n_columns/2. This is an error: If you are at the far left column and have a search radius that equals or exceeds the longitude range then you want all the columns to be inside, hence the upper limit must be n_columns. No tests affected but I ran into this with a grdseamount research case where I only used a small region to view a portion of a seamount whose radius exceeded half the width and things got oddly clipped...
Member
Author
|
Also a good example of a problem that does take a bit of testing and debugging to figure out, and in the end a single line of code is at fault (and have been so for years). |
Anso no point letting d_row exceed n_rows since it may add to allocation burden elsewhere. And the limit on x is also true for Cartesian of course.
Member
Author
|
Just note after that I wrote the summary on top I realized we can improve matters further:
Without these we might occasionally request too much memory for a filter, so might as well impose these limits. Again, no change to any tests. |
Member
Author
|
I guess it is Easter Sunday and only godless Norwegians are working. I will review this myself... |
Member
That's next Sunday. Today is the day when we go chase for wild orchids.
|
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
When we need to access all nodes inside a given search radius (as we do in grdfilter, nearneighbor, grdmask etc), we first determine limits on the number of rows and columns away from the search center. For geographic data this also means we consider the change in longitudinal distance with latitude. All this works fine. However, because for high latitudes (even ±90) we may divide by cos(lat) and hence we had a "safety valve" that clipped the upper limit of how many columns we may want to search to either side of a node to
n_columns/2. This is an error: If you are at the far left column and have a search radius that equals or exceeds the longitude range then you want all the columns to be inside, hence the upper limit must ben_columns.No tests affected but I ran into this with a grdseamount research case where I only used a small region to view a portion of a seamount whose radius exceeded half the width and things got oddly clipped in longitude but not in latitude...