-
Notifications
You must be signed in to change notification settings - Fork 56
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
Two failures in test_cube.R on PowerPC #408
Comments
|
Thanks for the bug report. I can certainly lower the comparison epsilon for that machine, or in general. It is a bit surprising that hasn't raised its head before -- this is not new code. Any idea? |
I guess that is a somewhat dated platform?
|
@eddelbuettel this is an old-school mac. |
Yes the 32-bit raised an eyebrow, the 10.8 even more. Decade old per https://en.wikipedia.org/wiki/OS_X_Mountain_Lion No reason for it to fail though. |
@eddelbuettel afaik macOS support for R 4.2.0 is official on High Sierra and above. The last supported version of R for PowerPC was v2.7.0 (c.f. Previous releases of R for Mac OS X |
@barracuda156 Can you paste in > str(.Machine)
List of 29
$ double.eps : num 2.22e-16
$ double.neg.eps : num 1.11e-16
$ double.xmin : num 2.23e-308
$ double.xmax : num 1.8e+308
$ double.base : int 2
$ double.digits : int 53
$ double.rounding : int 5
$ double.guard : int 0
$ double.ulp.digits : int -52
$ double.neg.ulp.digits : int -53
$ double.exponent : int 11
$ double.min.exp : int -1022
$ double.max.exp : int 1024
$ integer.max : int 2147483647
$ sizeof.long : int 8
$ sizeof.longlong : int 8
$ sizeof.longdouble : int 16
$ sizeof.pointer : int 8
$ sizeof.time_t : int 8
$ longdouble.eps : num 1.08e-19
$ longdouble.neg.eps : num 5.42e-20
$ longdouble.digits : int 64
$ longdouble.rounding : int 5
$ longdouble.guard : int 0
$ longdouble.ulp.digits : int -63
$ longdouble.neg.ulp.digits: int -64
$ longdouble.exponent : int 15
$ longdouble.min.exp : int -16382
$ longdouble.max.exp : int 16384
> str(.Platform)
List of 8
$ OS.type : chr "unix"
$ file.sep : chr "/"
$ dynlib.ext: chr ".so"
$ GUI : chr "X11"
$ endian : chr "little"
$ pkgType : chr "source"
$ path.sep : chr ":"
$ r_arch : chr ""
> |
@coatless @eddelbuettel Yes, R upstream does not support older macOS (perhaps no hardware to test), but the latest R works fine on 10.5+, and we support those systems in Macports. ppc32 also exists on *BSD, Linux and AIX; I think at least several BSDs support it in their latest OS versions (probably some Linux too). |
Yeah I am pretty sure we still build / used to build on these machines to. My current grid is https://buildd.debian.org/status/package.php?p=r-cran-rcpparmadillo and looks pretty green: But I as maintainer made an executive decision here that is different from many of my fellow Debian maintainers and I do not run CI on these as I find CRAN sufficiently paranoid. So if any one of those machines had a funky 12 1/2 bit FPU I would not know ... based on the CRAN packages alone. The R package itself of course runs the equivalent of |
@eddelbuettel Here is what I got on a PowerMac (native ppc32):
|
@eddelbuettel And this in Rosetta (10.6.8 with building for ppc):
Well, looks identical, but just for the record. (I test on both systems. 10.5.8 only occasionally.) |
Hm. Only difference is the shorter |
Perhaps irrelevant to the case, but other difference is that on ppc32 |
@eddelbuettel If I can assist in any way to find a fix for this, please let me know. |
As your log showed, this failed in two tests in one file, both using a particular equality threshold. If we look even more closely we see that that threshold is already lowere for Windoze so maybe we just need to reflect your situation. Please try this diff: modified inst/tinytest/test_cube.R
@@ -24,7 +24,7 @@ library(RcppArmadillo)
Rcpp::sourceCpp("cpp/cube.cpp")
.onWindows <- .Platform$OS.type == "windows"
-critTol <- if (.onWindows) 1.0e-6 else 1.5e-7
+critTol <- if (.onWindows || .Machine$sizeof.long == 4) 1.0e-6 else 1.5e-7
## test arrays
dbl_cube <- array(1.5:27.5, rep(3, 3)) |
@eddelbuettel Thank you! I have an update: it seems that my fix to
|
Just to be explicit you did NOT change the sensitivity of the two previous tests which now pass after you took care of If so congrats but please close this. Seems like there never was an issue. |
@eddelbuettel Yes, I ran tests prior to changing anything, and they pass now. Thank you for your help! |
|
Hello, Same exacly error on FreeBSD fixed with a dirty change: --- inst/tinytest/test_cube.R.orig 2023-02-19 18:01:50 UTC
+++ inst/tinytest/test_cube.R
@@ -23,8 +23,9 @@ Rcpp::sourceCpp("cpp/cube.cpp")
Rcpp::sourceCpp("cpp/cube.cpp")
-.onWindows <- .Platform$OS.type == "windows"
-critTol <- if (.onWindows) 1.0e-6 else 1.5e-7
+#.onWindows <- .Platform$OS.type == "windows"
+#critTol <- if (.onWindows || .Machine$sizeof.long == 4) 1.0e-6 else 1.5e-7
+critTol <- 1.0e-6
## test arrays
dbl_cube <- array(1.5:27.5, rep(3, 3)) Is there a better way to fix? R on FreeBSD reports:
Thanks |
You may be able to use |
Something like:
|
Maybe |
Ok, understand it. I'm maintaining FreeBSD port https://www.freshports.org/math/R-cran-RcppArmadillo/ and this is the first time that I'm using tests. I will need to patch wisely since port is builded in: aarch64, amd64, armv6, armv7, i386, powerpc, powerpc64 and powerpc64le arches. |
And I am of course in favour of ports, and tests. But "sometimes things differ" and it may just be best to keep a local directory Your "wise patch" would be appreciated because what had been suggested so far is a global change for all builds everywhere all the time which is surely a little more far-reaching and why I hesitate which I hope you understand. |
@nunotexbsd If you come up with an upstreamable patch, please make it OS-agnostic, since we would want this to work correctly on other systems which run on |
Ok, I will make tests on amd64/i386 aarch64/armv7 for a start. Thanks! |
On my systems I've noted that long is:
amd64/aarch64: Tier 1 and according to https://stat.ethz.ch/R-manual/R-devel/library/base/html/zMachine.html:
Anyway, I decided to commit port update with patch as a start: .onWindows <- .Platform$OS.type == "windows"
-critTol <- if (.onWindows) 1.0e-6 else 1.5e-7
+critTol <- if (.onWindows || .Machine$sizeof.long == 4 || .Machine$sizeof.long == 8) 1.0e-6 else 1.5e-7 to match FreeBSD arches and fix tests. It was nice that a global check is done here to all supported systems but it is ok too to fix it with patch in our ports. Open to sugestions :) Thanks |
The || between 4 and 8 means that just about every machine will now have the lowered threshold. Not something I would distribute to all users of RcppArmadillo but if that is what you think you need to do I am glad you have closure. You could alternatively also just document that a test is behaving differently and skip it (via the patch). I do that sometimes for projects and specific settings. |
@nunotexbsd If the fix is not meant for the upstream, you could just apply the patch to the threshold conditionally on the arch. |
In case upstream would fix it globally, patch provided is to indicate that FreeBSD is fixed that way. |
If by condition you mean in the source code, then for macOS it is So, for example, to have a specific code for ppc32 on macOS, this can be used:
I do not know what conditions are used in FreeBSD makefiles in ports. |
@nunotexbsd One well-known repo for |
The text was updated successfully, but these errors were encountered: