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

Improve BPCells compatibility (JackStraw + RunPCA) #8271

Merged
merged 3 commits into from
Dec 19, 2024

Conversation

bnprks
Copy link

@bnprks bnprks commented Jan 6, 2024

These are some changes brought about from this BPCells issue, then referenced in Seurat issue #8267

  1. Make the JackStraw function compatible with BPCells objects. This approach shuffles the matrix subset as a dense matrix, but leaves the remainder of the data on-disk. For best performance the BPCells object should be stored in row-major order so that the matrix subsets can be performed efficiently, but it will work either way.
  2. Improve performance of RunPCA by using the BPCells svds function. This takes advantage of the same solver RSpectra uses, but at the C++ level to avoid overheads of repeatedly creating BPCells C++ objects on each solver iteration. It also avoids the pitfall that irlba can only use its fast C version with in-memory matrices and otherwise falls back to a slower pure-R implemenation.

@dcollins15
Copy link
Contributor

Apologies @bnprks! My intent was to merge this in on the heels of #8966 but got pulled off in another direction. Circling back now as we prepare for the next CRAN release 🙃 The good news is that with CI checks now properly configured, we should be much more responsive to pull requests going forward.

These changes LGTM 🚀

This will be the next PR to merge, but before that, we'll need to:

  • Rebase bnprks:develop onto satijalab:develop.
  • Update the changelog.
  • Bump the package version (5.1.0.9011 -> 5.1.0.9012).

If you're keen to tackle these updates, feel free to push them up; otherwise, I'll take care of it shortly.

Thanks, as always, for the new features and for your patience as we integrate them 🙏

@bnprks
Copy link
Author

bnprks commented Dec 19, 2024

Hi @dcollins15, glad to hear you're putting in a new CRAN release! I made the changes you requested, though feel free re-word the changelog message or edit anything else as needed -- I don't mind my branch getting edited even with a force push since I'm just using the branch for this PR

@dcollins15 dcollins15 merged commit ab5374e into satijalab:develop Dec 19, 2024
2 checks passed
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

Successfully merging this pull request may close these issues.

2 participants