Repository navigation
Add contigs input to savana/run + minor version bump - #13108
manascripts wants to merge 2 commits into
Conversation
erikrikarddaniel
left a comment
There was a problem hiding this comment.
Reviewed with Claude Code.
Thanks, the contigs input itself looks good and matches savana/to. My comments are about the tests:
- The contigs test can't fail. The test genome (
genome.fasta) has onlychr22, and the list given ischr22, so the restriction is a no-op. The nanopore snapshot is unchanged from master, where no list was passed. A test where the list actually excludes something would show the option is honoured. One option: a list naming a contig that has no reads, then check the output is empty. I haven't tried whether SAVANA accepts that. - The default path is no longer tested. Every test now passes a list, so nothing runs without
--contigs(inline suggestion). - The pacbio test now runs the nanopore data and gives an identical snapshot (inline question).
- Minor:
meta.ymlcould say the file has one contig name per line (inline suggestion).
|
The tests have been changed according to the suggestions - empty outputs are asserted for the test where the target contig (chr1) is not in present in the input |
erikrikarddaniel
left a comment
There was a problem hiding this comment.
Reviewed with Claude Code.
Thanks, the earlier points are addressed. The chr1 test now shows the contigs list is honoured: I dropped --contigs locally and it fails on variantCount=28. All four tests pass with docker.
Remaining, all minor (inline):
- The chr22 test still has a snapshot identical to "no contig", so it cannot fail on the restriction. I'd drop it.
- In the chr1 test,
assert process.out.findAll { ... }is true for any non-empty map. Snapshotting it pins the version like the other tests. meta.yml: "when empty" reads as an empty file. "when not given" matches what the module handles.
| tag "savana/run" | ||
|
|
||
| test("homo_sapiens - nanopore") { | ||
| test("homo_sapiens - nanopore - chr22") { |
There was a problem hiding this comment.
This test still cannot fail on the restriction: genome.fasta only has chr22, so its snapshot is identical to the "no contig" one (28 variants, 16 read-support lines). The new chr1 test now covers that the list is honoured (I dropped ${contigs_arg} locally and it fails on variantCount=28), so this one only duplicates "no contig" and is the slowest test (~97 s here). I'd drop it, or keep it only if you want a case where the list matches the contig in the data.
| { assert file(process.out.sv_breakpoints_bedpe.get(0).get(1)).size() == 0 }, // BEDPE file is empty | ||
| { assert file(process.out.sv_breakpoints_read_support.get(0).get(1)).readLines().size() == 1 }, // read support file has only header line | ||
| { assert file(process.out.inserted_sequences.get(0).get(1)).size() == 0 }, // inserted sequences file is empty | ||
| { assert process.out.findAll { key, val -> key.startsWith('versions') } } |
There was a problem hiding this comment.
assert process.out.findAll { ... } is true for any non-empty map, so it never checks the version. The other tests snapshot it, which pins 1.3.8:
| { assert process.out.findAll { key, val -> key.startsWith('versions') } } | |
| { assert snapshot(process.out.findAll { key, val -> key.startsWith('versions') }).match() } |
That adds a small entry to the .snap. Also, the trailing // comments on lines 87 to 90 repeat what the assertion says and can go.
| e.g. `[ id:'contigs' ]` | ||
| - contigs: | ||
| type: file | ||
| description: Optional file listing the contigs to analyse, one contig per line. All contigs in the bam files are analysed when empty. |
There was a problem hiding this comment.
"when empty" reads as an empty file. What the module handles is the input not being given ([]); an empty file would be passed on as --contigs and I haven't checked what SAVANA does with it.
| description: Optional file listing the contigs to analyse, one contig per line. All contigs in the bam files are analysed when empty. | |
| description: Optional file listing the contigs to analyse, one contig per line. All contigs in the bam files are analysed when not given. |
This pull request updates the
savana/runandsavana/classifymodules to use version1.3.8and adds support for providing an optional contigs list file to restrict analysis to specific contigs. Corresponding tests andmeta.ymlare also updated.PR checklist
Closes #XXX
topic: versions- See version_topicslabelnf-core modules test <MODULE> --profile dockernf-core modules test <MODULE> --profile singularitynf-core modules test <MODULE> --profile condanf-core subworkflows test <SUBWORKFLOW> --profile dockernf-core subworkflows test <SUBWORKFLOW> --profile singularitynf-core subworkflows test <SUBWORKFLOW> --profile conda