Use non-exact matrix downsample test comparison - #129
Conversation
|
Oops, forgot about the downstream implications. A better solution would be for me to actually update |
Also added some more checks to ensure we don't lose integer precision.
These tests no longer make sense because the downsampling is no longer done on a per-event basis (i.e. single UMIs or reads in each count). Rather, the new algorithm operates on the entire count at once, for greater efficiency. This all means that UMI and read counts are no longer interchangeable, which means that the results from downsampleReads/Matrix are no longer directly comparable. This is okay - the test itself was highly contrived anyway as it only works when every UMI has a single read.
|
Just remove the affected test, there's no point changing it to test for correctness because the prior tests already do that. Also switched to the new hypergeometric sampler for |
|
Impressive as ever: I was just putting this up to run the tests before asking you what you thought, and here you are with the answer ready-baked. Though as you point out I should have read beyond the affected test! Cheers |
2 participants
scuttlecommit a997286 (March 27, 2026) rewrote downsampleMatrix() to use the tatami API, replacing thescuttle::downsample_vectorBernoulli loop withRf_rhyper()per gene per cell. The previous tests relied on both functions sharing the same RNG sequence under a fixed seed — that equivalence is now broken.Proposed fix:
Replace the seed-coupled exact comparison with direct correctness checks on downsampleReads() output:
sum(Y) == round(prop * sum(X))forbycol=FALSE;colSums(Y) == round(prop * colSums(X))forbycol=TRUE(both guaranteed by no-replacement sampling)