Codecov Report

❗ No coverage uploaded for pull request base (master@bb9f50f). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #3364   +/-   ##
=========================================
  Coverage          ?   94.62%           
=========================================
  Files             ?       65           
  Lines             ?    13366           
  Branches          ?        0           
=========================================
  Hits              ?    12648           
  Misses            ?      718           
  Partials          ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb9f50f...fdabca5. Read the comment docs.

Codecov Report

❗ No coverage uploaded for pull request base (master@bb9f50f). Click here to learn what that means.
The diff coverage is n/a.

@@            Coverage Diff            @@
##             master    #3364   +/-   ##
=========================================
  Coverage          ?   94.74%           
=========================================
  Files             ?       65           
  Lines             ?    12149           
  Branches          ?        0           
=========================================
  Hits              ?    11510           
  Misses            ?      639           
  Partials          ?        0

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bb9f50f...38c947e. Read the comment docs.

Changes amended, R mean is compared vs constant values, same frollmean is compared against constant values. Based on that comparison we either replace NaN to NA or leave as is.

As Martin wrote in #17441 :
"See ?NA, which says
Numerical computations using NA will normally result in NA: a possible exception is where NaN is also involved, in which case either might result (which may depend on the R platform). "

Given this, why not make frollmean return a consistent result which does not depend on the platform? In other areas of data.table we have departed from base R where it makes sense to. I don't see why you are trying to match identically the platform-specific behavior. Or, relax these tests to accept NA or NaN when mixed NA and NaN inputs are present. I don't think anyone needs to distinguish NA from NaN in the output of frollmean when mixed NA, NaN are present in the input. Especially when R doesn't guarantee a particular result in this case anyway.

Put another way ... maybe_NaN_to_NA() is used in tests which test frollmean(). But maybe_NaN_to_NA() itself contains calls to frollmean() and switches depending on the behavior observed. That's a circle that's too complicated in a test environment. It should be cleaner and simpler.

There are only two tests that are actually failing on CRAN Solaris: 9999.123 & 9999.162.

test(9999.123, identical(frollmean(x, n, algo="exact"), ma(x, n)))
test(9999.162, identical(frollmean(x, n, algo="exact", adaptive=TRUE), ama(x, n)))

These fail because mixed NA and NaN are in the input for these, iiuc.
Could these two be relaxed to :

test(9999.123, frollmean(x, n, algo="exact"), ma(x, n))
test(9999.162, frollmean(x, n, algo="exact", adaptive=TRUE), ama(x, n))

this way, test() will consider them equal because all.equal considers NA and NaN the same. You can still retain stricter identical() call in the many other tests.

This would be a 2 line change (plus comment to each) rather than the +29-14 change of the PR currently.

Given this, why not make frollmean return a consistent result which does not depend on the platform?

It will cost performance. We outsource that part to code generated by compiler, same as R.
Quoting Tomas Kalibera on why R cannot have consistent behaviour on that:

Yes, the performance overhead of fixing this at R level would be too
large and it would complicate the code significantly. The result of
binary operations involving NA and NaN is hardware dependent (the
propagation of NaN payload) - on some hardware, it actually works the
way we would like - NA is returned - but on some hardware you get NaN or
sometimes NA and sometimes NaN. Also there are C compiler optimizations
re-ordering code

maybe_NaN_to_NA() itself contains calls to frollmean() and switches depending on the behavior observed. That's a circle that's too complicated in a test environment. It should be cleaner and simpler.

maybe_NaN_to_NA actually doesn't call frollmean but depends on data.table handling of NaN-NA. We have to detect somehow how data.table behaves on that matter. Introducing new function or using gmean would only complicate more, that is why I used frollmean. It can be cleaner and simpler, as you proposed, but then we are switching off those two exactness (NaN-NA and rounding correction) test for every environment, not only for those where compilation flags or platform specifics differs. Anyway this is minor issue so will apply your suggestion.

failing on CRAN Solaris

Failed froll tests are not failing CRAN status in 1.12.0. There was no stop in the script if some tests failed. This is now changed and on 1.12.2 submission we will be able to see how Solaris and other platforms there handles that.

This would be a 2 line change (plus comment to each) rather than the +29-14 change of the PR currently.

Yes, those two are failing now, but on other platforms we could get failures on others amended. Using maybe_NaN_to_NA was a safe way to test this wherever possible, otherwise treat NaN as NA.

maybe_NaN_to_NA actually doesn't call frollmean

Here's the line that does: 38c947e#diff-83559ac036a3b9d070fc219734cf6d39L447

Yes, those two are failing now, but on other platforms we could get failures on others amended. Using maybe_NaN_to_NA was a safe way to test this wherever possible, otherwise treat NaN as NA.

But the root cause is that identical() is being passed to test() in many of the froll tests. That's too strict. test() is designed to compare x to y allowing for tolerance using all.equal(). all.equal() considers NA and NaN to be the same. When there's a difference between x and y, test() prints the outputs that differ and the output of all.equal (which can be a helpful message sometimes) and we see that in the logs. When you call identical() yourself and pass that to test(), that's overriding the intention of test(): it's too strict and we lose the diagnostics.

$1.6 of R-exts contains this bullet point at the end :

Only test the accuracy of results if you have done a formal error analysis. Things such as checking that probabilities numerically sum to one are silly: numerical tests should always have a tolerance. That the tests on your platform achieve a particular tolerance says little about other platforms. R is configured by default to make use of long doubles where available, but they may not be available or be too slow for routine use. Most R platforms use ‘ix86’ or ‘x86_64’ CPUs: these may use extended precision registers on some but not all of their FPU instructions. Thus the achieved precision can depend on the compiler version and optimization flags—our experience is that 32-bit builds tend to be less precise than 64-bit ones. But not all platforms use those CPUs, and not all83 which use them configure them to allow the use of extended precision. In particular, ARM CPUs do not (currently) have extended precision nor long doubles, and long double was 64-bit on HP/PA Linux.
If you must try to establish a tolerance empirically, configure and build R with --disable-long-double and use appropriate compiler flags (such as -ffloat-store and -fexcess-precision=standard for gcc, depending on the CPU type84) to mitigate the effects of extended-precision calculations.

We are not supposed to use identical(), see what breaks on CRAN and then change it according to platform differences. That's a lot of work for us and impacts CRAN maintainers too. We're supposed to use all.equal() in the first place, as test() does built-in.

Thanks for relaxing these tests. Much simpler and clearer now. Will merge. Let's come back to the other tests that use identical().

Here's the line that does: 38c947e#diff-83559ac036a3b9d070fc219734cf6d39L447

I explicitly put that outside of function definition to use that switch as constant defined based on base R compilation flag and data.table compilation flag, and/or platform.

identical()

I explained in #3371

see what breaks on CRAN

As mentioned above. froll tests were not raising exception on fails, so none of current CRAN failures is because of froll tests. Do we have access to tests/froll.Rout from CRAN checks? then I could see in output if any of tests failed.

edit: I misunderstood. Yes, seeing what breaks on CRAN and fixes that would be pain for us and maintainers.