Repository navigation
Severus fix - #223
Merged
Merged
Severus fix#223
Conversation
Build severus_input from PHASING_HAPLOTYPING's haplotagged BAMs instead of the unphased minimap2 BAMs, so Severus (and Wakhan, which reads the same channel) see the HP tags. SAVANA already built this channel itself; it now reuses severus_input. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| def sv_meta_keys = ['id', 'paired_data', 'platform', 'sex', 'fiber', | ||
| 'clair3_model', 'clairS_model', 'clairSTO_model', 'kinetics'] | ||
|
|
||
| PHASING_HAPLOTYPING.out.tumor_normal_hapbams_ch |
There was a problem hiding this comment.
Snapshot is not name-only for Severus. The PR description says "snapshots list Severus outputs by name only, so no snapshot changes expected". That's wrong. tests/.nftignore only ignores Severus's read_ids.csv, severus.log and the *.vcf.gz/.tbi files. breakpoints_double.csv, read_qual.txt and {all_SVs,somatic_SVs}/breakpoint_clusters*.tsv are md5-checked in tests/default.nf.test.snap (lines 612–634), and default.nf.test is tagged small.
Now that Severus gets HP-tagged reads, the per-haplotype support in breakpoints_double.csv should change. One of two things will happen:
- the md5s drift and the
smallCI test fails until the snapshot is regenerated, or - they don't drift, which suggests the test data doesn't show the fix working.
Please regenerate tests/default.nf.test.snap and explain the Severus md5 changes in the PR. Also regenerate the extended snapshots (clair_only, consensus, deep_only, union, chm13) that include Severus outputs.
Comment on lines
907
to
909
| // Attach phased germline VCF (used by SEVERUS for phased SV calling) | ||
| .join(PHASING_HAPLOTYPING.out.phased_germline_vcf) | ||
| .set{severus_input} |
There was a problem hiding this comment.
Missing test coverage for the fix, and Wakhan's input change is untested.
- No test checks the behaviour this PR fixes. That bug was
severus.logreportingH1 / H2 coverage 0.0 / 0.0and every hVAF beingx,0,0.default.nf.testonly checks thatseverus_somatic.vcf.gzexists, andseverus.logis in.nftignore. If someone switchesseverus_inputback to the unphased BAMs, CI would still pass. Please add an assertion to thesmalldefault test, e.g. thatsample1/variants/severus/severus.logreports non-zero H1/H2 coverage, or that some Severus record has a non-zero haplotype VAF. - Wakhan now gets the haplotagged BAMs too, but
conf/test.configsetsskip_wakhan = true. The onlysmalltest that enables Wakhan (sv_annotation_compose.nf.test) is a preview run that only checks process names. So no CI test runs Wakhan on the new input. Per REVIEW.md, please run the Slurm test array (or a real--skip_wakhan falserun) and report the result before merging.
|
Automated review: 2 findings. The channel logic looks correct to me. It's the same branch/join that SAVANA used before, and the hapbam names (
A human reviewer needs to run: the |
|
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.
Summary
severus_inputis now built fromPHASING_HAPLOTYPING.out.tumor_normal_hapbams_ch(the Longphase-haplotagged BAMs) rather than the unphased minimap2 BAMs, so Severus gets HP tags for haplotype-aware SV calling. Before this,severus.logreportedMedian coverage by PASS reads (H1 / H2 / H0): 0.0 / 0.0 / Nand every hVAF wasx,0,0.severus_input, so it now gets the haplotagged BAMs too.severus_input, which removes the duplicated block.somatic_smallvar_input(ClairS / DeepSomatic) is unchanged.Test plan
nextflow lint workflows/lrsomatic.nf(no new errors)-profile test,singularity -stub: aborts withJoin mismatch ... key=[id:sample1, paired_data:true, ...]before phasing. Unmodifieddev(91b8c1e) aborts the same way, so this is not caused by this PR, but it means the stub run can't check the new channel end to end🤖 Generated with Claude Code