Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for running CoGAPS as a standalone Nextflow pipeline, handling both .h5ad and .rds inputs and exposing all parameters via the CLI (including multi-pattern support as comma-separated values).
- Updated
nextflow.configdefaults (including newn_top_genes) and resource limits. - Enhanced
main.nfto select top genes, refine distributed execution logic, and define a full example workflow for.h5ad/.rds. - Updated documentation (
README.md), R package version, CI ignore rules, and added.cirroconfigs.
Reviewed Changes
Copilot reviewed 9 out of 11 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| nextflow.config | Tweaked default params, added n_top_genes, increased resources |
| main.nf | Gene selection, distributed params logic, workflow channels |
| README.md | Added Nextflow usage example and CLI param table |
| .Rbuildignore | Updated ignore rules for Nextflow, work/, main.nf, and .cirro |
Comments suppressed due to low confidence (5)
README.md:38
- The
inputandoutdirparameters are missing from this list. Adding them will help users understand how to specify input folders and output directory.
Supported CLI params and their defaults are:
main.nf:3
- The process label was changed to 'process_high' but there is no corresponding config block defining this label. Ensure 'process_high' is defined in nextflow.config or revert to a matching label.
label 'process_high'
main.nf:62
- The closing brace here no longer matches an opening
ifsince the guard was removed, causing a syntax error. Either restore theif (!is.null(dist_param))wrapper or remove this stray}.
};
main.nf:64
- Hardcoding
outputFrequency = 100ignores the user-specifiedniterations. Consider restoringfloor($cparams.niterations/10)to align output frequency with iterations.
outputFrequency = 100);
main.nf:119
- This label was updated from 'process_low' to 'process_medium', but no matching config block appears to exist. Verify the label mapping in nextflow.config.
label 'process_medium'
| ch_patterns = Channel.from(patterns) | ||
|
|
||
| //example channel with cparams | ||
| ch_fixed_params = Channel.of([niterations: params.niterations, sparse: params.sparse, distributed: params.distributed, nsets:params.nsets, nthreads:1]) |
There was a problem hiding this comment.
nthreads is statically set to 1, so any CLI override of --nthreads will be ignored. Use params.nthreads instead of a hardcoded value.
| ch_fixed_params = Channel.of([niterations: params.niterations, sparse: params.sparse, distributed: params.distributed, nsets:params.nsets, nthreads:1]) | |
| ch_fixed_params = Channel.of([niterations: params.niterations, sparse: params.sparse, distributed: params.distributed, nsets:params.nsets, nthreads: params.nthreads]) |
There was a problem hiding this comment.
nthreads is tricky (works with multithreaded only) and never used, would prefer leaving it hardcoded
drbergman
left a comment
There was a problem hiding this comment.
Can't say I can understand all the details of the changes here, but I think supporting easily running cogaps with multiple pattern numbers is worthy of incrementing the minor version.
Would it also be worth it to support range expression for npatterns? e.g. npatterns=5:5:20? A function to parse this string should be straightforward.
| @@ -1,5 +1,5 @@ | |||
| Package: CoGAPS | |||
| Version: 3.27.4 | |||
| Version: 3.27.5 | |||
There was a problem hiding this comment.
Certainly this PR is worthy of a minor increment!
.h5adand.rdsfiles