Add meta maps to cellranger ref modules and downstream consumers#12257
Open
nictru wants to merge 5 commits into
Open
Add meta maps to cellranger ref modules and downstream consumers#12257nictru wants to merge 5 commits into
nictru wants to merge 5 commits into
Conversation
Propagate meta through cellranger/mkgtf and cellranger/mkref, update cellranger/count and cellranger/multi to accept meta-tagged references, and add an input/output filename collision check to cellranger/mkgtf. Generated by Composer
Align module test assertions with the newer nf-core pattern for clearer failures when a process does not complete successfully. Generated by Composer
LouisLeNezet
reviewed
Jul 7, 2026
LouisLeNezet
left a comment
LouisLeNezet
left a comment
Contributor
There was a problem hiding this comment.
Thanks for cleaning it up.
I just have a few comments.
On a broader scale, do you know if the input tuple should be rearranged in bigger tuple ? (see my comment for multi)
| { assert process.success }, | ||
| { | ||
| assert snapshot( | ||
| process.out.versions_cellranger, |
Contributor
There was a problem hiding this comment.
If outs doesn't have too many file, could you use sanitizeOutput(process.out, unstableKeys:["outs"]) instead of this whole assertion ?
| assert process.success | ||
| assertAll( | ||
| { assert process.success }, | ||
| { assert snapshot(process.out).match() } |
Contributor
There was a problem hiding this comment.
You could add sanitizeOutput here also
| assert process.success | ||
| assertAll( | ||
| { assert process.success }, | ||
| { assert snapshot(process.out).match() } |
| assert process.success | ||
| assertAll( | ||
| { assert process.success }, | ||
| { assert snapshot( |
| test("homo_sapiens - identical input output names - stub") { | ||
|
|
||
| options "-stub" | ||
| config "./nextflow-collision.config" |
Contributor
There was a problem hiding this comment.
Please use a common nextflow.config and use params.module_prefix instead.
| assertAll( | ||
| { assert process.success }, | ||
| { assert snapshot( | ||
| path( "${outputDir}/homo_sapiens_reference/fasta" ), |
| assertAll( | ||
| { assert process.success }, | ||
| { assert snapshot( | ||
| process.out, |
Simplify assertions with sanitizeOutput for simple outputs, use versions topic snapshots with selective file checks for complex modules, and configure mkgtf collision testing via params.module_prefix. Generated by Composer
Declare a default module_prefix param so ext.prefix is valid in CI, where Nextflow 25.10.2 rejects undefined params in process config. Generated by Composer
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
2 participants
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.
It's always annoying in pipelines if some modules expect reference files with meta maps, and other ones without. Leads to a lot of
mapoperators to ensure the right channel shape. This PR adds meta map inputs to cellranger modules, that were previously lacking them