Open
Conversation
Initial gpu impl
There was a problem hiding this comment.
Pull request overview
This pull request introduces GPU acceleration support for the propr package through CUDA implementation, along with significant refactoring of the codebase architecture and enhanced testing infrastructure.
Changes:
- Added CUDA GPU acceleration for core computational functions (lrv, lrm, omega, ctzRcpp, comparison operations)
- Refactored C++ code into modular CPU/GPU dispatch architecture with separate interface, kernel, and trait layers
- Enhanced testing with permutation options (feature-wise vs sample-wise), additional test coverage, and GPU-specific tests
- Added profiling infrastructure (NVTX markers) and CI/CD improvements with containerized testing
Reviewed changes
Copilot reviewed 154 out of 160 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| thirdparty/Catch2 | Added Catch2 testing framework as git submodule |
| tests/testthat/test-SHARED-updatepermutes.R | Added test for permutation conservation of gene/sample-wise totals |
| tests/testthat/test-SHARED-updateCutoffs-propr.R | Updated tests to use sample-wise permutation option and skip parallel tests on CI |
| tests/testthat/test-SHARED-updateCutoffs-propd.R | Added skip for parallel test on local CI |
| tests/testthat/test-SHARED-lrv.R | Added GPU vs CPU comparison test for lrv function |
| tests/testthat/test-SHARED-getCutoff-propr.R | Split test into feature-wise and sample-wise variants |
| tests/testthat/test-SHARED-getCutoff-propd.R | Updated tests to use non-moderated F-statistics |
| tests/testthat/test-SHARED-getAdjacency-propr.R | Fixed adjacency matrix logic to use directional comparison instead of absolute value |
| tests/testthat/test-SHARED-getAdjacency-propd.R | Enhanced test diagnostics and updated to use non-moderated F-statistics |
| tests/testthat/test-RCPP-ctzRcpp.R | Added GPU variant test for zero frequency counting |
| tests/testthat/test-RCPP-count.R | Added GPU variant tests for comparison operations |
| tests/testthat/test-PROPR-clr-alr-vlr.R | Added CPU vs GPU comparison tests for clr and alr transformations |
| tests/testthat/test-PROPD-weight.R | Refactored weight tests with new fetch_weights helper and comprehensive weight validation |
| tests/testthat/test-PROPD-updateF.R | Added comprehensive tests for F-statistic calculation with and without moderation |
| tests/testthat/test-PROPD-theta.R | Updated weight calculation to use sample reliability weights from limma |
| tests/testthat/test-PROPD-lrm-lrv.R | Added GPU vs CPU numerical equivalence test for propd |
| tests/testthat/test-GRAFLEX.R | Added GPU variant test for FDR calculation and updated permutation test |
| tests/testthat/test-GET-getMatrix.R | Enhanced test diagnostics with detailed error messages |
| tests/tests-gpu/test-PROPR-clr-alr-vlr.R | Added GPU-specific test suite for transformations |
| src/omega.cpp | Refactored to use dispatch pattern with CPU/GPU backend selection |
| src/lrv.cpp | Refactored to use dispatch pattern with CPU/GPU backend selection |
| src/lrm.cpp | Refactored to use dispatch pattern with CPU/GPU backend selection |
| src/lr2propr.cpp | Refactored to use dispatch pattern with CPU/GPU backend selection |
| src/host_profiler.cpp | Implemented host-side profiling infrastructure |
| src/host_exclusive_profiler.cpp | Implemented exclusive profiling to prevent nested measurements |
| src/dispatch/device_selector.cpp | Implemented runtime GPU backend selection based on R options |
| src/dispatch/cuda/omega.cu | CUDA implementation of omega calculations |
| src/dispatch/cuda/lrm.cu | CUDA implementation of log-ratio mean calculations |
| src/dispatch/cuda/lr2propr.cu | CUDA stubs for proportionality transformations |
| src/dispatch/cuda/ctzRcpp.cu | CUDA implementation of zero frequency counting |
| src/dispatch/cuda/comparison.cu | CUDA implementation using Thrust for comparison operations |
| src/dispatch/cuda/CMakeLists.txt | Build configuration for CUDA dispatch sources |
| src/dispatch/cpu/*.cpp | CPU implementations refactored into dispatch pattern |
| src/ctzRcpp.cpp | Refactored interface with GPU support |
| src/cpp_tests/*.cu | Added C++ unit test infrastructure |
| src/comparison.cpp | Refactored interface with GPU support |
| src/backend.h | Removed old header (functionality moved to dispatch pattern) |
| src/Makefile | Added minimal makefile |
| src/CMakeLists.txt | Main build configuration with CUDA support |
| scripts/git/pre-push | Added pre-push hook for automated testing |
| old_vignettes/e_differential.Rmd | Minor formatting correction |
| man/*.Rd | Updated documentation for new parameters and corrected examples |
| inst/include/propr/**/*.{h,hpp,cuh} | Added extensive header infrastructure for GPU/CPU implementations |
| configure | Added configuration script for CMake-based build |
| cmake/*.cmake | Added CMake modules for finding R, Rcpp, and testthat |
| ci/run-tests.R | Added CI test runner script |
| bench/nvbench | Added nvbench submodule for benchmarking |
| README.md | Fixed NULL to NA in examples and added local development section |
| R/*.R | Added NVTX profiling markers throughout R code |
| R/3-shared-updatePermutes.R | Added permutation_option parameter support |
| R/1-propr.R | Added permutation_option parameter and slot |
| R/1-propr-OOP.R | Added permutation_option slot to propr class |
| R/10-nvtx-shim.R | Added NVTX profiling shim functions |
| OLDCMakeLists.txt | Legacy CMake configuration preserved |
| NEWS.md | Updated changelog with new features |
| DESCRIPTION | Added nvtxR to suggests, fixed formatting |
| CMakeLists.txt | Main CMake configuration with CUDA detection |
| .gitmodules | Added submodule configuration |
| .gitignore | Added build and CI artifact directories |
Comments suppressed due to low confidence (4)
tests/testthat/test-RCPP-count.R:1
- Missing space after comma before '4' in the expected value.
src/dispatch/cpu/omega.cpp:1 - The function
pad_matrix2has an unclear suffix '2'. Consider renaming to something more descriptive likepad_matrixor explain why the '2' suffix is necessary.
configure:1 - Misspelling in variable name 'ABSOLUE_PATH'. Should be 'ABSOLUTE_PATH'.
inst/include/propr/interface/backend.hpp:1 - Parameter name 'nfeatsbool' is confusing - it appears to be an integer representing number of features but has 'bool' in the name. Should be renamed to 'nfeats'.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| #' | ||
| #' @export | ||
| logratio <- function(counts, ivar, alpha) { | ||
| logratio <- function(counts, ivar, alpha=NA) { |
There was a problem hiding this comment.
Missing space around the default parameter assignment. Should be alpha = NA for consistency with R style guidelines.
Suggested change
| logratio <- function(counts, ivar, alpha=NA) { | |
| logratio <- function(counts, ivar, alpha = NA) { |
Comment on lines
+219
to
+220
| ## For Local Developement | ||
| For local developement install the pre-push script `./scripts/git/pre-push` to run the tests. No newline at end of file |
There was a problem hiding this comment.
Misspelling of 'development' as 'developement'.
Suggested change
| ## For Local Developement | |
| For local developement install the pre-push script `./scripts/git/pre-push` to run the tests. | |
| ## For Local Development | |
| For local development install the pre-push script `./scripts/git/pre-push` to run the tests. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.