Skip to content
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

Small inconsistency in the lmBenchmark.R file? #126

Open
statzhero opened this issue Jun 12, 2023 · 4 comments
Open

Small inconsistency in the lmBenchmark.R file? #126

statzhero opened this issue Jun 12, 2023 · 4 comments

Comments

@statzhero
Copy link

In the lmBenchmark.R file I read

    ## LDLt Cholesky decomposition with rank detection
    exprs["LDLt"] <- alist(RcppEigen::fastLmPure(mm, y, 2L))

but in the documentation for RcppEigen::fastLm the method is described as

an integer scalar with value [...] 3 for the LDLT Cholesky,

Is this correct?

> sessionInfo()
R version 4.2.0 (2022-04-22)
Platform: aarch64-apple-darwin20 (64-bit)
Running under: macOS 13.3.1

Matrix products: default
LAPACK: /Library/Frameworks/R.framework/Versions/4.2-arm64/Resources/lib/libRlapack.dylib

locale:
[1] en_US.UTF-8/en_US.UTF-8/en_US.UTF-8/C/en_US.UTF-8/en_US.UTF-8

attached base packages:
[1] stats     graphics  grDevices utils     datasets  methods  
[7] base     

other attached packages:
[1] bench_1.1.3         RcppEigen_0.3.3.9.3 speedglm_0.3-5     
[4] biglm_0.9-2.1       DBI_1.1.3           MASS_7.3-58.1      
[7] Matrix_1.5-3       
@eddelbuettel
Copy link
Member

eddelbuettel commented Jun 12, 2023

It could be a mismatch. The enum that drives is 0-based (in fastLm.cpp)

    enum {ColPivQR_t = 0, QR_t, LLT_t, LDLT_t, SVD_t, SymmEigen_t, GESDD_t}; 

so that would LLT be 2 and LDLT be 3. The actualy branching is in in the static inline function do_lm() just below. You could add a print statement to be sure... This is all code originally written by @dmbates which has stood some tests of time but it is always possibly that these two got crossed.

@statzhero
Copy link
Author

statzhero commented Jun 12, 2023

Is it possible that the documentation is behind? It doesn't seem to mention:

    ## SVD using the Lapack subroutine dgesdd and Eigen support
    exprs["GESDD"] <- alist(RcppEigen::fastLmPure(mm, y, 6L))

@eddelbuettel
Copy link
Member

[ NNB: You could make this easier by stating which files you take snippets from or use the GitHub feature of a link with an anchor. ]

From what I just quoted, the enum for the GESDD method is six, this line calls with six. Seems correct to me.

@statzhero
Copy link
Author

statzhero commented Jun 12, 2023 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants