search

bwlewis / irlba

Fast truncated singular value decompositions
127 stars 17 forks source link

Generalized code to non-vector matrix products. #45

Closed LTLA closed 5 years ago

LTLA commented 5 years ago

This coerces the product of A %*% VJ into a vector, regardless of its original class. This generalizes the behaviour for Matrix classes with an "x" slot (presumably dgCMatrix instances?) to other matrix representations - for example, we've been using it with DelayedArray objects.

codecov-io commented 5 years ago

Codecov Report

Merging #45 into master will increase coverage by 0.05%. The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #45      +/-   ##
==========================================
+ Coverage   89.03%   89.09%   +0.05%     
==========================================
  Files           8        8              
  Lines         885      880       -5     
==========================================
- Hits          788      784       -4     
+ Misses         97       96       -1
Impacted Files Coverage Δ
R/irlba.R 85.22% <100%> (+0.06%) :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 2f3233a...7aa20f9. Read the comment docs.

LTLA commented 5 years ago

Nudge. I guess it's holiday season now...

bwlewis commented 5 years ago

Sorry for the delay, have been pre-occupied. WIll work on this this week...

On 12/12/18, Aaron Lun notifications@github.com wrote:

Nudge. I guess it's holiday season now...

-- You are receiving this because you are subscribed to this thread. Reply to this email directly or view it on GitHub: https://github.com/bwlewis/irlba/pull/45#issuecomment-446486058

LTLA commented 5 years ago

Post-new years nudge...

bwlewis commented 5 years ago

Thanks for this, apologize for long delay -- have been away.

A CRAN update is desperately needed now...

bwlewis commented 5 years ago

By the way, that weird code was some ancient performance optimization. Now, no longer really needed since csparse matrices are in the C code path anyway.

LTLA commented 5 years ago

Thanks @bwlewis. Looks like you read my mind - I was going to nudge this yesterday (and then I fell asleep).

Looking forward to the CRAN update, I've got a couple of packages downstream waiting for it.

LTLA commented 5 years ago

Nudge - any timeframe for the CRAN update? If there are outstanding issues, I can try to help out.

bwlewis commented 5 years ago

I'm trying to get it ready...hopefully by tomorrow (Tuesday). Stuck on two prcomp_irlba bugs: #47 and #34. These need to be fixed...

LTLA commented 5 years ago

I'm on it. #47 looks easier so I'll deal with that one first.

bwlewis commented 5 years ago

finally sent off 2.3.3 to CRAN...hopefully will go OK. I'll let you know if there are further problems.

On 1/28/19, Aaron Lun notifications@github.com wrote:

I'm on it. #47 looks easier so I'll deal with that one first.

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/bwlewis/irlba/pull/45#issuecomment-458174010

bwlewis commented 5 years ago

Argh. Despite passing on my systems, it failed some checks on CRAN and was rejected. Fixing now. Will re-submit today. The error is in an ssvd example, here:

https://win-builder.r-project.org/incoming_pretest/irlba_2.3.3_20190205_051828/Debian/00check.log

A real puzzle for me. Something's always chaning in R... I skipped my running on winbuilder checklist step foolishly thinking since no C-code had changed then it would pass.

LTLA commented 5 years ago

While it runs fine on my computer, delta_u does become a 2-by-2 matrix after the crossprod, so I can see why the build machine complained. Was the delta_u update meant to be a sum(diag(...)) or something? Don't know enough about the theory to really help you here, I'm afraid.

bwlewis commented 5 years ago

Yeah, just realized the problem. It is in fact a bug in ssvd, but indeed passed on my systems too.

Fixing.

On 2/5/19, Aaron Lun notifications@github.com wrote:

While it runs fine on my computer, delta_u does become a 2-by-2 matrix after the crossprod, so I can see why it complained. Was this meant to be a sum(diag(...)) or something? Don't know enough about the theory to really help you here, I'm afraid.

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/bwlewis/irlba/pull/45#issuecomment-460643269

bwlewis commented 5 years ago

Yeah, just realized the problem. It is in fact a bug in ssvd, but indeed passed on my systems too.

Submitting to winbuilder/devel first to check it works there.

Thanks so much for all your help with this package!

I am preparing a road map for a major new release. This will outline current known problems with irlba (the algorithm, not the software), and some proposed improvements. I'll let you know when that is ready (it will be in the repository).

LTLA commented 5 years ago

Great - happy to chip in, with testing if nothing else.

bwlewis commented 5 years ago

OK, the 2.3.3 source package is on CRAN now: https://cran.r-project.org/web/checks/check_results_irlba.html

It takes a few days for the binary packages to appear...