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

Tolerance applies in a surprising way to large integers #138

Open
hadley opened this issue Mar 12, 2022 · 8 comments
Open

Tolerance applies in a surprising way to large integers #138

hadley opened this issue Mar 12, 2022 · 8 comments
Labels
feature a feature request or enhancement

Comments

@hadley
Copy link
Member

hadley commented Mar 12, 2022

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
Copy link
Member Author

hadley commented Mar 12, 2022

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.

@MichaelChirico
Copy link
Contributor

MichaelChirico commented Mar 13, 2022

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().

@hadley
Copy link
Member Author

hadley commented Mar 13, 2022

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?

@MichaelChirico
Copy link
Contributor

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?

@hadley
Copy link
Member Author

hadley commented Mar 14, 2022

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
Copy link
Member Author

hadley commented Mar 14, 2022

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
Copy link
Member Author

hadley commented Mar 14, 2022

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.

@MichaelChirico
Copy link
Contributor

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)

@hadley hadley added the feature a feature request or enhancement label Mar 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature a feature request or enhancement
Projects
None yet
Development

No branches or pull requests

2 participants