search

mlcollyer / RRPP

RRPP: An R package for fitting linear models to high-dimensional data using residual randomization
https://mlcollyer.github.io/RRPP/
4 stars 1 forks source link

Possible misuse of `Matrix::qrR` #9

Closed jaganmn closed 1 year ago

jaganmn commented 1 year ago

Hello,

Your package, RRPP, has:

https://github.com/mlcollyer/RRPP/blob/2e296bfe220421295c78040df1d60052e922658d/R/lm.rrpp.r#L665 https://github.com/mlcollyer/RRPP/blob/2e296bfe220421295c78040df1d60052e922658d/R/RRPP.support.code.r#L773

but qrR may be deprecated in favour of qr.R as soon as Matrix 1.6-0 (to be released in July), although to give package maintainers more time we may delay the deprecation to a later version.

To be backwards compatible with earlier versions of Matrix, I would patch your code like so:

R <- if(isS4(object) && is.unsorted(j <- object@q)) qr.R(object)[, order(j), drop = FALSE] else qr.R(object)

rather than simply replacing qrR with qr.R. Let me know if you think that you could make such a change before July, or if you think that you need more time. Well, in any case, it would be good to verify that your package passes its checks under Matrix 1.6-0, which you can install with install.packages("Matrix", repos="http://R-Forge.R-project.org").

Thanks,

Mikael { Matrix package co-author }

jaganmn commented 1 year ago

Having said that, it is not clear to me why you are only back-permuting in the sparse case. Note that qr.R(<qr>) gives R where A P2 = Q R, whereas qrR(<sparseQR>) gives R P2' where P1 A P2 = Q R. So maybe a patch is required independently of Matrix 1.6-0. :-)

I.e., if you want R in both cases, then do:

R <- qr.R(object)

and if you want R P2' in both cases, then do:

j <- if(isS4(object)) object@q else object$pivot
R <- if(is.unsorted(j)) qr.R(object)[, j, drop = FALSE] else qr.R(object)
mlcollyer commented 1 year ago

Dear Mikael,

Thank you for the heads up! Indeed my use of qrR was part of a kludge intended for finding the best route to find linear model coefficients, based on whether a linear model design matrix could be considered sparse. I would not expect you to delve into the history of the package development, but using the Matrix library is still a fairly recent development for our package, and it was spurred by cases on long computation time using the dense-matrix calculations in R base functions. (Our package tends to perform linear model fits thousands of times, so having a proficient way of doing that if linear model design matrices are sparse has been extremely helpful!) The solutions I used that you pointed out were biased by already having similar solutions in the dense-matrix case, before attempting to use Matrix functions. These solutions surely could be improved and I will look into improvements based on your suggestion.

Regarding timing, please do not let me hold you up with your planned package improvements. We are also in the middle of some updates to our package, so I am currently already in the weeds with coding. A fresh minor package release for RRPP will be well-timed with one for Matrix.

As a side-note, although I am still not fully comfortable with all of the Matrix bells and whistles, this package has been really instrumental for our ability to offer fairly fast permutation-based solutions to RRPP users with large data problems. I really appreciate the work the Matrix team does!

With warm regards, Mike

On Jun 15, 2023, at 8:23 PM, Mikael Jagan @.***> wrote:

Having said that, it is not clear to me why you are only back-permuting in the sparse case. Note that qr.R() gives R where A P2 = Q R, whereas qrR() gives R P2' where P1 A P2 = Q R. So maybe a patch is required independently of Matrix 1.6-0. :-)

— Reply to this email directly, view it on GitHub https://github.com/mlcollyer/RRPP/issues/9#issuecomment-1593535130, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABUNU4UZ7BYURKZSIRLHTJLXLNHIXANCNFSM6AAAAAAZIGY3II. You are receiving this because you are subscribed to this thread.

jaganmn commented 1 year ago

After more complete rev. dep. checking and discussion, we've decided to not go ahead with the deprecation. I jumped the gun a bit with this issue, but maybe that's OK, if indeed there was a mistake in the original qrR usage. I've expanded the Matrix documentation w. r. t. factorizations quite a bit for Matrix 1.6-0 - hopefully that will help alleviate some of the discomfort going forward.

I'll leave the issue open under a more accurate title - feel free to close when ready.

mlcollyer commented 1 year ago

I was reminded that one of the reasons for the code I used was to simply not have a warning returned, and I followed the suggestion of the warning that was returned when using qr.R to use qrR. I can use your recent suggestions along with suppressWarnings() to make sure my code is updated to use qr.R rather than qrR, which means our package should be more ready when you do indeed deprecate qrR. Please be aware that since we are updating our package in other regards, it might take some time before such changes are updated on CRAN.

mlcollyer commented 1 year ago

Closing now. Thanks for the helpful hints!

jaganmn commented 1 year ago

Yes, the method for qr.R did warn somewhat unnecessarily, in my opinion, hence in Matrix 1.6-0 there is no warning (but rather much better documentation).

Having said that, it has been possible to disable the warning in earlier versions of Matrix simply by setting options(Matrix.quiet.qr.R = TRUE). That was documented in help("sparseQR-class"), but I don't fault you for missing it ...

I'd recommend using the options approach instead of suppressWarnings, to avoid accidentally suppressing useful warnings.

mlcollyer commented 1 year ago

Hi Mikael,

I did not miss the note in the help documentation but the warning, itself, encouraged use of the qrR function, which neither provoked a warning nor caused any issues in the results we evaluated. Therefore, I was content with the solution. I think the documentation was clear for how the function differed from the base qr.R function, but at least for cases I evaluated results, I obtained consistent results between qrR (with default arguments) and qr.R, the former without warning. I appreciate, however, after this discussion that it was not an ideal solution and will be happy to make updates.

Cheers! Mike

On Jun 16, 2023, at 9:45 PM, Mikael Jagan @.***> wrote:

Yes, the method for qr.R did warn somewhat unnecessarily, in my opinion, hence in Matrix 1.6-0 there is no warning (but rather much better documentation).

Having said that, it has been possible to disable the warning in earlier versions of Matrix simply by setting options(Matrix.quiet.qr.R = TRUE). That was documented in help("sparseQR-class"), but I don't fault you for missing it ...

I'd recommend using the options approach instead of suppressWarnings, to avoid accidentally suppressing useful warnings.

— Reply to this email directly, view it on GitHub https://github.com/mlcollyer/RRPP/issues/9#issuecomment-1595204526, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABUNU4QMA2P2UUIMQUOB5PTXLSZVZANCNFSM6AAAAAAZIGY3II. You are receiving this because you modified the open/close state.