39 errors in projectr method for sparse matrix data #42
Conversation
dimalvovs
commented
Sep 26, 2024
dimalvovs
commented
- implement projectR on sparseMatrix by calling the dense one in chunks;
- fix missing LinearEmbeddingMatrix class definition
- refactor
| NP=NA, # vector of integers indicating which columns of loadings object to use. The default of NP=NA will use entire matrix. | ||
| full=FALSE, # logical indicating whether to return the full model solution. By default only the new pattern object is returned. | ||
| family="gaussianff" # VGAM family function (default: "gaussianff") | ||
| NP=NULL, # vector of integers indicating which columns of loadings object to use. The default of NP=NA will use entire matrix. |
There was a problem hiding this comment.
also, it's noteworthy to me that the default for NP is NA above and NULL here. Not sure if it's desired. just noting it
There was a problem hiding this comment.
yup, because is.na(NP) was never going to work if NP is specified:
> NP <- c(1,3,5)
> if(!is.na(NP)) {print("it's a bug")}
Error in if (!is.na(NP)) { : the condition has length > 1
is.null works fine:
> is.null(NP)
[1] FALSE
see line 85 too - I changed the check from is.NA to is.null too
tests/testthat/test_projectR.R
Outdated
|
|
||
| pdense <- projectR(dense, loadings, full=TRUE) | ||
| psparse <- projectR(sparse, loadings, full=TRUE) | ||
| expect_identical(projectR(dense, loadings), |
There was a problem hiding this comment.
Don't you want to test that pdense and psparse are identical? Not sure why here (and above) you compute projectR twice for both dense and sparse, but at least here you want full=TRUE inside the expect_identical call, yeah?
There was a problem hiding this comment.
exactly, thanks for spotting and that's why we need reviews! fixed in c3fdb7a
There was a problem hiding this comment.
As far as I can see, this PR simplifies the code and makes it more readable. I can't quite tell what's being removed in R/projectR.R L104-126, but I'm guessing the call on new L106 handles that now. I left a couple comments throughout the changes.
Also, the PR message suggests that the LinearEmbeddingMatrix class def is being fixed. I can't find a place where it is being changed, let alone fixed.
|
@drbergman thanks very much for the review, |
dimalvovs
deleted the
39-errors-in-projectr-method-for-sparse-matrix-data
branch
