Fix broken bwa hybrid alignment; resolve hybrid coverage design - #3
Open
DanielSprockett wants to merge 1 commit into
Open
DanielSprockett wants to merge 1 commit into
DanielSprockett wants to merge 1 commit into
Conversation
…tation The bwa hybrid path was broken: _merge_long_reads_bwa ran minimap2 through the _run helper, which captures and discards stdout, so the long-read SAM was never written and the subsequent open(long.sam) raised FileNotFoundError. Hybrid short+long samples under a bwa aligner therefore always failed. - Route the long-read alignment through _align_minimap2_preset, which redirects minimap2's SAM to long.sam, then append its records (header skipped) to the short SAM — producing one merged BAM per sample as intended. Dropped the unused aligner_bin/samtools_bin params; minimap2 binary is now a parameter. - Resolve the hybrid coverage design question: keep the merged-BAM total-depth approach (one BAM per sample, CoverM once; relative cross-sample signal preserved because every sample is processed identically). Reject the previously-documented read-count-weighted-mean merge, which needs separate per-technology CoverM passes for no benefit to relative coverage. - CLAUDE.md corrected: merging is at the alignment stage, not "in coverage.py weighted by read count"; the rejected alternative and the one real reason to revisit it (per-technology identity filtering) are noted. Tests: test_align_hybrid.py covers the merge (long records appended, header not duplicated) and is a regression guard for the discarded-stdout bug. 136 pass.
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
1 participant
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.
Stacked on #2 (base
pipeline-hardening). Resolves the hybrid coverage question from the code review (§1.2).The two problems
_merge_long_reads_bwaran minimap2 via the_runhelper, which captures and discards stdout, so the long-read SAM was never written; the next lineopen(long.sam)raisedFileNotFoundError. Any hybrid (short+long) sample under a bwa aligner failed.Resolution
_align_minimap2_preset(which redirects minimap2's SAM to disk), then append its records to the short SAM — one merged BAM per sample as intended.Tests
test_align_hybrid.py: the merge appends long records and doesn't duplicate the header; a regression guard for the discarded-stdout bug. 136 pass.