Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds Nextflow pipeline enhancements to the CoGAPS package, including improved resource configuration, container updates, and preprocessing capabilities. The changes modernize the Nextflow workflow with better resource limits, updated container versions, and a new preprocessing step for data filtering.
- Adds a new COGAPS_PREPROCESS process for data filtering and gene selection
- Updates container versions and resource configurations in nextflow.config
- Fixes resource allocation patterns and adds proper resource limits
- Includes comprehensive unit tests for distributed CoGAPS functionality
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| nextflow.config | Updates resource limits, removes check_max calls, and increases default memory/CPU allocations |
| main.nf | Adds preprocessing step, updates container versions, reorganizes stub/script sections |
| tests/testthat/test_DistributedCogaps.R | New unit tests for single-cell and genome-wide distributed modes |
| R/DistributedCogaps.R | Bug fixes for accessing fixed patterns in distributed processing |
| Dockerfile | Adds sparseMatrixStats dependency |
| DESCRIPTION | Version bump and date update |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
|
|
||
| # copy P matrix - same for all sets | ||
| Pmean <- result[[1]]@sampleFactors | ||
| Pmean <- result[[1]]@metadata$params@fixedPatterns |
There was a problem hiding this comment.
Both genome-wide and single-cell modes are using @fixedPatterns for different matrices. In genome-wide mode, line 236 should likely use @sampleFactors instead of @fixedPatterns for the P matrix, and in single-cell mode, line 258 should use @featureLoadings for the A matrix.
There was a problem hiding this comment.
I don't think so.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
drbergman
left a comment
There was a problem hiding this comment.
Far as I can tell, it looks all in order. Just the one package version print to check on. Were some of these added checks to ensure that we don't return an array of all 0's?
| @@ -1,5 +1,5 @@ | |||
| Package: CoGAPS | |||
| Version: 3.27.5 | |||
| Version: 3.29.1 | |||
There was a problem hiding this comment.
I assume this version bump makes sense?
There was a problem hiding this comment.
Yes, Bioconductor is strict about versions: in x.y.z, y is always odd in devel and even in release. Current release is 28, so current devel must be 29. Although ir may be 29.0, will find out when pushing to Bioconductor.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Yes, the added tests check for all 0's |
No description provided.