LTLA commented 5 years ago

The first commit fixes the following behaviour in the tiny problem case:

set.seed(0)
A <- matrix(runif(10), 5, 4)
center <- runif(4)
scale <- runif(4)

library(irlba)
X <- irlba(A, nv=3, nu=3, center=center, scale=scale)
X$d
# [1] 4.4874209 2.7501384 0.5115381
A2 <- scale(A, center=center, scale=scale)
ref <- irlba(A2, nv=3, nu=3)
ref$d
# [1] 4.2779403 1.4405317 0.2149879

The fix ensures that the first call gives the same results as the second reference call.

The second commit fixes the problem described in #42; the first call is now the same as the second.

Some points:

Probably worth putting these checks for equality in the unit tests somewhere.
The fastpath=TRUE case for #42 is not fixed, I didn't want to mess with the C code.

Session information

``` R Under development (unstable) (2018-11-02 r75535) Platform: x86_64-pc-linux-gnu (64-bit) Running under: Ubuntu 16.04.5 LTS Matrix products: default BLAS: /home/cri.camres.org/lun01/Software/R/trunk/lib/libRblas.so LAPACK: /home/cri.camres.org/lun01/Software/R/trunk/lib/libRlapack.so locale: [1] LC_CTYPE=en_GB.UTF-8 LC_NUMERIC=C [3] LC_TIME=en_GB.UTF-8 LC_COLLATE=en_GB.UTF-8 [5] LC_MONETARY=en_GB.UTF-8 LC_MESSAGES=en_GB.UTF-8 [7] LC_PAPER=en_GB.UTF-8 LC_NAME=C [9] LC_ADDRESS=C LC_TELEPHONE=C [11] LC_MEASUREMENT=en_GB.UTF-8 LC_IDENTIFICATION=C attached base packages: [1] stats graphics grDevices utils datasets methods base other attached packages: [1] irlba_2.3.3 Matrix_1.2-15 loaded via a namespace (and not attached): [1] compiler_3.6.0 grid_3.6.0 lattice_0.20-38 ```

lintr-bot commented 5 years ago

R/ssvd.R:201:51: style: Remove spaces before the left parenthesis in a function call.

    lambda <<- vapply(seq(length(p)), function(j) (1 - alpha) * z[p[j], j] + alpha * z[p[j] + 1, j], pi, USE.NAMES=FALSE)
                                                  ^

lintr-bot commented 5 years ago

R/ssvd.R:201:51: style: Remove spaces before the left parenthesis in a function call.

    lambda <<- vapply(seq(length(p)), function(j) (1 - alpha) * z[p[j], j] + alpha * z[p[j] + 1, j], pi, USE.NAMES=FALSE)
                                                  ^

codecov-io commented 5 years ago

Codecov Report

Merging #44 into master will decrease coverage by 0.02%. The diff coverage is 77.77%.

@@           Coverage Diff           @@
##           master   #44      +/-   ##
=======================================
- Coverage   89.02%   89%   -0.03%     
=======================================
  Files           8     8              
  Lines         875   882       +7     
=======================================
+ Hits          779   785       +6     
- Misses         96    97       +1

Impacted Files	Coverage Δ
R/irlba.R	`85.15% <77.77%> (+0.01%)`	:arrow_up:

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 e88eb3a...48802c3. Read the comment docs.

lintr-bot commented 5 years ago

R/ssvd.R:201:51: style: Remove spaces before the left parenthesis in a function call.

    lambda <<- vapply(seq(length(p)), function(j) (1 - alpha) * z[p[j], j] + alpha * z[p[j] + 1, j], pi, USE.NAMES=FALSE)
                                                  ^

LTLA commented 5 years ago

Nudging this PR. Any feedback - good? Bad? Ugly?

bwlewis commented 5 years ago

Sorry for the long delay! Thanks for this, this is great!

I'm merging and then will add the corresponding FASTPATH=TRUE fix.

Best,

Bryan

LTLA commented 5 years ago

Thanks @bwlewis. Would it be worth adding scale/center tests for the tiny problem case as well?

bwlewis commented 5 years ago

Yes @LTLA , that's a good call. I'll add those too.

bwlewis / irlba

Bug fixes for scaling arguments #44

Codecov Report