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

fix for bilinearInt NaN handling #693

Merged
merged 2 commits into from
Jul 7, 2022
Merged

Conversation

terrry-sh
Copy link
Contributor

Hey guys,
I've picked up an error in the handling of NaNs in the bilinearInt method for the extract method.
See the example below:

r <- terra::rast(nrow = 2, ncol = 2, nlyrs = 1, xmin = -100, xmax = 100, ymin = -100, ymax = 100)
terra::values(r) <- c(100, 100, NaN, 100)
print(terra::extract(r, data.frame(X = 45.0, Y = 45.0), method = "bilinear")$lyr.1)
# ought to be 100 but the value will be 55

Here the expected bilinear interpolation between points all with value 100 should always stay at 100 but we dip down to 55, 145. A quick peek at the source code shows that the handling isn't linearly adjusting when a NaN is found and simply using the value 0.5 which can lead to problems say when (y - y2) happens to be very small or very large; see below

terra/src/extract.cpp

Lines 168 to 183 in 1820db4

if (intx & inty) {
double d = dx * dy;
if (!(n1 || n2)) {
w11 = v11 * ((x2 - x) * (y - y2)) / d;
w12 = v12 * ((x - x1) * (y - y2)) / d;
} else {
w11 = n1 ? 0.0 : 0.5 * v11;
w12 = n2 ? 0.0 : 0.5 * v12;
}
if (!(n3 || n4)) {
w21 = v21 * ((x2 - x) * (y1 - y)) / d;
w22 = v22 * ((x - x1) * (y1 - y)) / d;
} else {
w21 = n3 ? 0.0 : 0.5 * v21;
w22 = n4 ? 0.0 : 0.5 * v22;
}

I've implemented in the first commit a few tests which fail on the current version of extract with method = "bilinear" and then in the second commit I've added the linear/bilinear factor back in to fix this issue. All tests pass and personally I've been using this algorithm on rasters at work with good results.

@rhijmans
Copy link
Member

rhijmans commented Jul 7, 2022

Thank you very much. I have merged it (but have not had time to give it a good look)

@terrry-sh
Copy link
Contributor Author

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants