joshuaulrich / xts

Extensible time series class that provides uniform handling of many R time series classes by extending zoo.
http://joshuaulrich.github.io/xts/
GNU General Public License v2.0
219 stars 70 forks source link

Unit test failure when running on i386 #384

Closed tillea closed 1 year ago

tillea commented 1 year ago

Description

I've updated the xts Debian package to the latest version (0.12.2). Debian builds and tests these packages automatically on different architectures. With the upload of version 0.12.2 the package fails on i386 as it is reported in Debian bug tracking system. A full build log is linked there. The errors are starting with

...
Loading required package: zoo

Attaching package: 'zoo'

The following objects are masked from 'package:base':

    as.Date, as.Date.numeric

Error in make.index.unique.xts(x, eps = eps) :
  (converted from warning) index value is unique but will be replaced; it is less than the cumulative epsilon for the preceding duplicate index values
In addition: Warning messages:
1: In make.index.unique.xts(x, eps = 1e-06) :
  index value is unique but will be replaced; it is less than the cumulative epsilon for the preceding duplicate index values
2: In make.index.unique.xts(x, eps = eps) :
  index value is unique but will be replaced; it is less than the cumulative epsilon for the preceding duplicate index values
Error in try.xts(sample.data.frame[, 1]) :
  Error in as.xts.double(x, ..., .RECLASS = TRUE) :   order.by must be either 'names()' or otherwise specified
Error in diff.xts(x, 1L, -1L) :
  'diff.xts' defined only for positive lag and differences arguments
Error in diff.xts(x, 1L, "a") : 'differences' must be integer
In addition: Warning message:
In diff.xts(x, 1L, "a") : NAs introduced by coercion
Error in diff.xts(x, -1L, 1L) :
  'diff.xts' defined only for positive lag and differences arguments
Error in diff.xts(x, "a", 1L) : 'lag' must be integer
In addition: Warning message:
In diff.xts(x, "a", 1L) : NAs introduced by coercion
Error in endpoints(x, on = "years", k = 0) : 'k' must be > 0
Error in endpoints(x, on = "years", k = -1) : 'k' must be > 0
Error in endpoints(x, on = "quarters", k = 0) : 'k' must be > 0

Expected behavior

I would expect that the tests would pass also on i386 as it was the case for version 0.12.1.

Minimal, reproducible example

Just run the test on some i386 machine

Kind regards, Andreas.

joshuaulrich commented 1 year ago

Thanks for the report Andreas! I'll take care of it. The rest of this information is for future me. I don't expect you to act on it.

All these "errors" are in the log because they're thrown as part of tests to check that errors are thrown when they should be. This version of the 'xts' package uses the 'RUnit' package for unit tests. 'RUnit' doesn't muffle errors when they're expected, so the log is cluttered with "Error in ..." messages. You have to look at the Unit Test Summary to identify any failing tests.

The relevant failing test is test.split_returns_named_list. This test was added in 0.12.2, so there is no regression from 0.12.1.

------------------- UNIT TEST SUMMARY ---------------------

RUNIT TEST PROTOCOL -- Sat Nov 12 17:18:54 2022 
*********************************************** 
Number of test functions: 431 
Number of errors: 0 
Number of failures: 1 

1 Test Suite : 
xts unit testing - 431 test functions, 0 errors, 1 failure
FAILURE in test.split_returns_named_list: Error in checkIdentical(nm_target, nm_ms, msg) : FALSE 
 microsecond data split by milliseconds
Error: 

unit testing failed (#test failures: 1, #R errors: 0)

My hypothesis is that the test fails on i386 because of precision issues. Here's a minimal example:

nm_minutes <- c("1970-01-01 00:00:00", "1970-01-01 00:01:00")
t1 <- as.POSIXct(nm_minutes[1], tz = "UTC")
us <- seq(1e-4, 2e-1, 1e-4)
x_us <- xts::xts(seq_along(us), t1 + us)
nm_ms <- names(split(x_us, "milliseconds"))
nm_target <- format(t1 + seq(0, 0.2, 0.001), "%Y-%m-%d %H:%M:%OS3")
RUnit::checkIdentical(nm_target, nm_ms, msg)
tillea commented 1 year ago

Hi, is there any progress in this? Regarding Debian release status its a bit urgent since we will freeze soon. Kind regards, Andreas.

joshuaulrich commented 1 year ago

Yikes, I'm sorry. I thought I took care of this...

joshuaulrich commented 1 year ago

I was able to replicate this on the i386/debian:latest docker container. Now this specific test only runs on 64-bit systems.

I'll start testing reverse-dependencies today, and should have an updated xts on CRAN by early next week.

Thanks again for the report!

tillea commented 1 year ago

Thanks for doing so.

tillea commented 1 year ago

Any success to track down the issue? I do not want to sound impatiently but the release process is creating some presure here at my side and some reverse dependencies of xts are affected.

AdrianBunk commented 1 year ago

@tillea I've got it passing by compiling R itself with -ffloat-store to mitigate the excess precision of the x87 FPU. I'll reassign the Debian bug with a patch in a while.

tillea commented 1 year ago

Am Thu, Jan 19, 2023 at 06:24:15AM -0800 schrieb Adrian Bunk:

@tillea I've got it passing by compiling R itself with -ffloat-store to mitigate the excess precision of the x87 FPU. I'll reassign the Debian bug with a patch in a while. So you think that rather r-base should be compiled with -ffloat-store? I realised that this is needed in some r-cran-* packages (like for instance https://salsa.debian.org/r-pkg-team/r-cran-igraph/-/blob/master/debian/patches/i386.patch and others) I tried r-cran-xts with this option but this did not worked.

joshuaulrich commented 1 year ago

This is fixed in the development version on GitHub. I'm working on getting it released to CRAN as soon as possible. I ran into some reverse-dependency issues with other changes and I'm trying to fix/revert them.

AdrianBunk commented 1 year ago

@tillea Confirmed by my testing is that the difference between failing and passing autopkgtest of r-cran-xts is whether r-base-core is from Debian unstable or from the same sources recompiled with -ffloat-store.

-ffloat-store would cost some performance for FPU code in R on i386, but given that it's 2023 this should not matter much since i386 is mainline vintage hardware (multiarch usecases shouldn't apply here).

I would guess this change might also help with similar issues in other packages, but that is just a guess.

tillea commented 1 year ago

Am Thu, Jan 19, 2023 at 06:49:24AM -0800 schrieb Adrian Bunk:

@tillea Confirmed by my testing is that the difference between failing and passing autopkgtest of r-cran-xts is whether r-base-core is from Debian unstable or from the same sources recompiled with -ffloat-store.

-ffloat-store would cost some performance for FPU code in R on i386, but given that it's 2023 this should not matter much since i386 is mainline vintage hardware (multiarch usecases shouldn't apply here).

I fully agree that users could live with performance issues on i386 since those who need performance will not use i386.

I would guess this change might also help with similar issues in other packages, but that is just a guess.

I'd agree with reassigning this to r-base.

tillea commented 1 year ago

Am Thu, Jan 19, 2023 at 06:41:37AM -0800 schrieb Joshua Ulrich:

This is fixed in the development version on GitHub. I'm working on getting it released to CRAN as soon as possible. I ran into some reverse-dependency issues with other changes and I'm trying to fix/revert them. Thanks for working on this anyway. As the discussion with Adrian has shown we might change the build of R for i386 anyway since this might reduce the number of hard to predict test failures on i386 hardware also for other R packages.