Skip to content

Commit

Permalink
Fixes R CHECK warnings and problems on some platforms
Browse files Browse the repository at this point in the history
  • Loading branch information
@jkrijthe
jkrijthe committed Nov 1, 2018
1 parent 99e19e2 commit f3f4250
Show file tree
Hide file tree
Showing 3 changed files with 8 additions and 4 deletions.
3 changes: 1 addition & 2 deletions src/Makevars
View file
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
## Use the R_HOME indirection to support installations of multiple R version
## PKG_LIBS = `$(R_HOME)/bin/Rscript -e "Rcpp:::LdFlags()"` $(LAPACK_LIBS) $(BLAS_LIBS) $(FLIBS)
PKG_LIBS = $(LAPACK_LIBS) $(BLAS_LIBS) $(FLIBS) $(SHLIB_OPENMP_CFLAGS)
PKG_CFLAGS = $(SHLIB_OPENMP_CFLAGS)
PKG_LIBS = $(LAPACK_LIBS) $(BLAS_LIBS) $(FLIBS) $(SHLIB_OPENMP_CXXFLAGS)
PKG_CXXFLAGS = $(SHLIB_OPENMP_CXXFLAGS)

## As an alternative, one can also add this code in a file 'configure'
Expand Down
3 changes: 1 addition & 2 deletions src/Makevars.win
View file
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@

## Use the R_HOME indirection to support installations of multiple R version
## PKG_LIBS = $(shell "${R_HOME}/bin${R_ARCH_BIN}/Rscript.exe" -e "Rcpp:::LdFlags()") $(LAPACK_LIBS) $(BLAS_LIBS) $(FLIBS)
PKG_LIBS = $(LAPACK_LIBS) $(BLAS_LIBS) $(FLIBS) $(SHLIB_OPENMP_CFLAGS)
PKG_CFLAGS = $(SHLIB_OPENMP_CFLAGS)
PKG_LIBS = $(LAPACK_LIBS) $(BLAS_LIBS) $(FLIBS) $(SHLIB_OPENMP_CXXFLAGS)
PKG_CXXFLAGS = $(SHLIB_OPENMP_CXXFLAGS)
6 changes: 6 additions & 0 deletions tests/testthat/test_Rtsne.R
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,9 @@ test_that("Scaling gives the expected result", {

test_that("Manual distance calculation equals C++ distance calculation", {

# Does not work on 32 bit windows
skip_on_cran()

# Exact
set.seed(50)
tsne_matrix <- Rtsne(as.matrix(iris_unique[,1:4]),verbose=FALSE,
Expand Down Expand Up @@ -75,6 +78,9 @@ test_that("Accepts data.frame", {

test_that("OpenMP with different threads returns same result",{

# Does not work on windows
skip_on_cran()

set.seed(50)
tsne_out_df1 <- Rtsne(iris_unique[,1:4],dims=3,verbose=FALSE, is_distance = FALSE,
theta=0.1,pca=FALSE,max_iter=iter_equal,num_threads=1)
Expand Down

6 comments on commit f3f4250

@LTLA
Copy link
Contributor

@LTLA LTLA commented on f3f4250 Dec 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jkrijthe Were you able to figure out the cause for the check failure on 32-bit Windows? I'm getting related errors on the same platform for scater, probably due to failed check above.

@jkrijthe
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, unfortunately I was not, but indeed, if I remember correctly, this was only happening on 32-bit Windows. For this first test, the slight numerical differences might add up faster than on other platforms, but I did not have time or convenient access to 32-bit Windows (other than through r-hub) to thoroughly get into this. Would love to hear if you find out more.

@LTLA
Copy link
Contributor

@LTLA LTLA commented on f3f4250 Dec 21, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good, you were able to trigger the error via rhub. In that case, I'll give it a go tomorrow, it should be possible to at least narrow down the cause. Was it the exact or approximate algorithms (or both)?

@jkrijthe
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that I checked the emails again, I'm not sure whether I tried to debug it through the rhub builds or the winbuilder builds. Unfortunately I do not have the logs anymore to see whether it was the exact or inexact test that failed.

@LTLA
Copy link
Contributor

@LTLA LTLA commented on f3f4250 Dec 22, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm... it passes r-hub's windows devel without problems, while the release build is not being co-operative (just times out, probably a build system issue). From reading the code, my best guess is that the discrepancy is due to the distance calculations. Yours have the form:

double res=0;
/* 'for' loop across dimensions */
    res += (a[d] - b[d])*(a[d] - b[d]);

... while R's dist() does something like:

double res=0;
/* 'for' loop across dimensions */
    double dev=(a[d] - b[d]);
    res += dev * dev;

The former may use extended precision throughout the entire calculation, while the latter forces a round-off after the subtraction. This may be enough to introduce small differences in the distance calculations, possibly only for i386 depending on the compiler settings.

@jkrijthe
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. Thanks for looking into this.

Please sign in to comment.