waldo icon indicating copy to clipboard operation
waldo copied to clipboard

Tolerance applies in a surprising way to large integers

Open hadley opened this issue 2 years ago • 8 comments

library(waldo)

dt <- as.POSIXct("2016-07-18 16:06:00", tz = "UTC")
compare(dt, dt + 5)
#> `old`: "2016-07-18 16:06:00"
#> `new`: "2016-07-18 16:06:05"
compare(1468857960, 1468857965)
#> `old`: 1468857960
#> `new`: 1468857965

(tol <- testthat::testthat_tolerance())
#> [1] 1.490116e-08
compare(dt, dt + 5, tolerance = tol)
#> ✓ No differences
compare(1468857960, 1468857965, tolerance = tol)
#> ✓ No differences

Created on 2022-03-12 by the reprex package (v2.0.1)

Compared with all.equal() which has different behaviour for POSIXct:

library(waldo)

dt <- as.POSIXct("2016-07-18 16:06:00", tz = "UTC")
all.equal(dt, dt + 5)
#> [1] "Mean absolute difference: 5"
all.equal(1468857960, 1468857965)
#> [1] TRUE

Created on 2022-03-12 by the reprex package (v2.0.1)

I don't think the behaviour is correct in either case: the purpose of tolerance is to ignore minor differences that arise due to floating point error, not to ignore differences in integers that can be exactly represented in floating point.

cc @MichaelChirico

hadley avatar Mar 12 '22 17:03 hadley

I'm not sure how to resolve this; the all.equal() behaviour doesn't seem correct to me, but I don't know how to derive the correct behaviour.

hadley avatar Mar 12 '22 18:03 hadley

IMO the all.equal() behavior is correct. c.f.

dt1 <- as.POSIXct("2016-07-18 16:06:00", tz = "UTC")
dt2 <- as.POSIXct("1970-01-01 00:00:00", tz = "UTC")
all.equal(dt1, dt1 + 5)
# [1] "Mean absolute difference: 5"
all.equal(as.numeric(dt1), as.numeric(dt1 + 5))
# [1] TRUE
all.equal(dt2, dt2 + 5)
# [1] "Mean absolute difference: 5"
all.equal(as.numeric(dt2), as.numeric(dt2 + 5))
# [1] "Mean absolute difference: 5"

Time-since-UTC-epoch is an implementation detail to which we shouldn't over-anchor ourselves. Any timestamps that are 5 seconds apart should have the same output of compare().

MichaelChirico avatar Mar 13 '22 03:03 MichaelChirico

I agree that any of timestamps that are (say) >0.01s different should compare as equal. But I'd think that same principle should also apply to integers? (regardless of whether they're integers or doubles under the hood). Why should the tolerance for only POSIXct always be absolute?

hadley avatar Mar 13 '22 21:03 hadley

Because time is "interval data" which for which 0 is arbitrary; temperature should similarly get its own all.equal() method, if there's, say, a c("fahrenheit", "temperature") and c("celsius", "temperature") class.

(struggling to find something authoritative-looking about this, so not sure the term is universal, but here's one source)

For unclassed numerics, it's a significant figures issue, right?

MichaelChirico avatar Mar 14 '22 07:03 MichaelChirico

I don't think it's about significant digits as much as floating point differences like `sqrt(2)^2 == 2. And the similar issue when LAPACK or other low-level linear algebra functions might return very slightly different results on different platforms due to slight implementation differences.

My intuition (which might be wrong) is that the tolerance should really only affect very small numbers and very very large numbers (i.e. where the exponent is very large and the gap between representable floating point numbers is larger), but numbers in the middle shouldn't be affected. Maybe I'm expecting the tolerance to be applied to the mantissa? (assuming the exponents must be equal)

hadley avatar Mar 14 '22 14:03 hadley

I think I'm maybe thinking of ULP based comparison? https://bitbashing.io/comparing-floats.html has a good discussion.

https://codingnest.com/the-little-things-comparing-floating-point-numbers/ is also useful because it describes what Catch2 (a C++ testing framework does)

hadley avatar Mar 14 '22 14:03 hadley

I think overall this implies that waldo's current approach to tolerance is overly simplistic and will need to be redesigned (in conjunction with testthat) to allow difference classes to use different values. That will require some thought so I think I'll at least get the current release out before coming back to think about this.

hadley avatar Mar 14 '22 14:03 hadley

That makes more sense to me, thanks for elaborating & thanks for the links.

One lingering thing that gives me pause is a potential "idempotency" issue?

I'm thinking of large (x, y) gets generated by code as floats (& are subject to floating point imprecision), but then gets serialized to CSV and now look like integers to CSV readers. Should waldo::compare(x, y) be stable to this serialization?

(I'm not 100% sure it's a relevant concern -- it could be that the imprecision happens only at the sub-integer level for numbers in integer range)

MichaelChirico avatar Mar 15 '22 00:03 MichaelChirico