escape one frollsd tests for valgrind #7548
Conversation
| exact_NaN = isTRUE(capabilities()["long.double"]) && identical(as.integer(.Machine$longdouble.digits), 64L) | ||
| if (!exact_NaN) { | ||
| cat("\n**** Skipping 7 NaN/NA algo='exact' tests because .Machine$longdouble.digits==", .Machine$longdouble.digits, " (!=64); e.g. under valgrind\n\n", sep="") | ||
| cat("\n**** Skipping 8 NaN/NA algo='exact' tests because .Machine$longdouble.digits==", .Machine$longdouble.digits, " (!=64); e.g. under valgrind\n\n", sep="") |
There was a problem hiding this comment.
Maybe we could do something like nskip for foreign messages to avoid incrementing this manually
data.table/R/test.data.table.R
Line 336 in 7e9907f
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7548 +/- ##
==========================================
- Coverage 98.97% 98.96% -0.02%
==========================================
Files 87 87
Lines 16733 16737 +4
==========================================
+ Hits 16561 16563 +2
- Misses 172 174 +2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
inst/tests/froll.Rraw
Outdated
| y = c(1e8+2.980232e-8, 1e8, 1e8, 1e8) # CLAMP0 test | ||
| test(6001.731, frollvar(y, 3)[4L], 0) | ||
| test(6001.732, frollsd(y, 3)[4L], 0) | ||
| if (exact_NaN) test(6001.732, frollsd(y, 3)[4L], 0) |
There was a problem hiding this comment.
the other cases are all using algo='exact', so this would be the first case being skipped with algo='fast'
There was a problem hiding this comment.
it's also v. interesting that frollvar(y, 3)[4L], which per glancing the implementation only differs by applying sqrt(), does not have this numerical difference issue
There was a problem hiding this comment.
Yes, sqrt must be causing this rounding error
There was a problem hiding this comment.
We could possibly use all.equal here instead... Then no need to escape test
There was a problem hiding this comment.
Any idea why 2.980232e-8 was chosen? Maybe we could make that a function of .Machine$double.eps (assuming that changes on/off valgrind) or of .Machine$longdouble.digits?
There was a problem hiding this comment.
Looks like the CLAMP0 in the comment refers to something that was refactored...
c2c5bde#diff-6eb444de657f71e8b89b3d686c4ec70e4ecff4e33154481d0b1b416d09449f1eL1140
There was a problem hiding this comment.
2.980232e-8 was chosen because without clamp, it produced wrong results. AFAIR I used rnorm with e-16 or smth and pushed onto frollsd until I found the example (with 3 or 4 other candidates) because the code looked numerically unstable (hitting negative numbers).
There was a problem hiding this comment.
Thanks for the extra context, very helpful. I did a similar exercise (with rnorm(1e6, sd=1e-8) to be precise) and found that, absent the clamping added in #7361, we can trigger the negative result on valgrind with a different epsilon:
y = c(1e8-0.000000017353503836151923775668, 1e8, 1e8, 1e8)
frollvar(y, 3)
# [1] NA NA 0.000000e+00 -1.110223e-16But it does seem prohibitively difficult to come up with an epsilon that fails with and without valgrind, and I'm not sure there's anything but cleverness points to score there anyway.
There was a problem hiding this comment.
We could possibly use all.equal here instead... Then no need to escape test
wound up doing essentially this, but all.equal() won't do because the intent of the test is to ensure the result is strictly non-negative.
3 participants
Closes #7546
Haven't tested, but looking at
and
I think it should do