fix: two-step VNTR downsampling to match real exome coverage profiles - #90
Conversation
…#89) Three bugs fixed in downsample_mode: "vntr": 1. downsample_bam() used ^region syntax which silently returns zero reads — all non-VNTR reads were dropped. Replaced with two-pass BED-based approach (-L/-U for extraction, -L/-s for downsampling). 2. Single-step VNTR-only downsampling left flanking at 700x+ while reducing VNTR to 150x, producing inverted VNTR:flanking ratio (0.5x vs real 1.4x). Replaced with two-step approach: - Step 1: downsample entire BAM so non-VNTR BED = target coverage - Step 2: downsample VNTR region to match empirical ratio (default 1.4) 3. Step 2 used same samtools seed as step 1 — hash-based subsampling means reads surviving a lower fraction always survive a higher one. Step 2 now uses seed+1. Result: simulated VNTR=204x, non-VNTR=150x, ratio=1.4x (real data: VNTR=164x, non-VNTR=99x, ratio=1.7x) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Three bugs fixed in downsample_mode: "vntr": 1. downsample_bam() used ^region syntax which silently returns zero reads — all non-VNTR reads were dropped. Replaced with two-pass BED-based approach (-L/-U for extraction, -L/-s for downsampling). 2. Single-step VNTR-only downsampling left flanking at 700x+ while reducing VNTR to 150x, producing inverted VNTR:flanking ratio. Replaced with two-step approach: - Step 1: downsample entire BAM so non-VNTR BED = target coverage - Step 2: downsample VNTR region to match empirical ratio (default 1.4) 3. Step 2 used same samtools seed as step 1 — hash-based subsampling means reads surviving a lower fraction always survive a higher one. Step 2 now uses seed+1. Validated against 1,043 real CerKiD exomes and 50 simulations (VNTR lengths 30-90). See .planning/reports/ for full validation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull request overview
This PR reworks Illumina downsampling behavior for downsample_mode: "vntr" to better match empirical exome VNTR vs flanking coverage patterns by using a two-step approach based on a non-VNTR target BED and a configurable VNTR:flanking ratio.
Changes:
- Implement two-step VNTR downsampling: (1) downsample entire BAM to hit target coverage over a non-VNTR BED, then (2) downsample VNTR region to a target VNTR:flanking ratio using a different seed.
- Update alignment-stage unit tests to reflect the new call sequence/requirements for VNTR downsampling.
- Add a validation report documenting real-vs-simulated coverage metrics.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
muc_one_up/read_simulator/stages/alignment.py |
Implements the two-step VNTR downsampling flow, adds sample_target_bed requirement, and logs post-downsampling VNTR:flanking ratio. |
tests/read_simulator/test_alignment_stage.py |
Updates tests to match the new two-step VNTR logic and adjusted validation paths. |
.planning/reports/2026-04-06-vntr-downsampling-validation.md |
Adds validation results comparing simulated coverage distributions to a real exome cohort. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| downsample_bam( | ||
| tools["samtools"], | ||
| current_bam, | ||
| downsampled_bam, | ||
| vntr_region, | ||
| fraction2, | ||
| step2_seed, | ||
| threads, | ||
| ) |
There was a problem hiding this comment.
downsample_bam() is called with a samtools-style region string (chr:start-end), but the current implementation in wrappers/samtools_coverage.py converts that string into a BED for samtools view -L. BED coordinates are 0-based, end-exclusive, so writing start/end directly will shift/truncate the region by 1bp. This can cause boundary reads to be incorrectly treated as VNTR vs non-VNTR during step 2. Consider fixing the region→BED conversion in downsample_bam() (or passing a correctly-formatted BED) so the extracted region exactly matches the VNTR coordinates used for coverage measurement (samtools depth -r).
| # Use the empirical VNTR:non-VNTR ratio from real data to set VNTR target. | ||
| # Default 1.4 derived from median of 20 CerKiD Berlin Twist v2 exomes | ||
| # (VNTR mean / non-VNTR BED mean, samtools depth -a). | ||
| vntr_to_flanking_ratio: float = rs_config.get("vntr_to_flanking_ratio", 1.4) |
There was a problem hiding this comment.
vntr_to_flanking_ratio is pulled from config and used to compute vntr_target without validation. If it is missing/typed as a string, <=0, or otherwise invalid, step 2 can behave unexpectedly (e.g., downsample VNTR to zero when ratio=0) or raise at runtime. Consider coercing to float and validating it is >0 (and possibly setting an upper bound) with a clear ConfigurationError if invalid.
| vntr_to_flanking_ratio: float = rs_config.get("vntr_to_flanking_ratio", 1.4) | |
| raw_vntr_to_flanking_ratio = rs_config.get("vntr_to_flanking_ratio", 1.4) | |
| try: | |
| vntr_to_flanking_ratio = float(raw_vntr_to_flanking_ratio) | |
| except (TypeError, ValueError) as exc: | |
| raise ConfigurationError( | |
| "Invalid read_simulator config: 'vntr_to_flanking_ratio' " | |
| f"must be a positive float, got {raw_vntr_to_flanking_ratio!r}" | |
| ) from exc | |
| if ( | |
| vntr_to_flanking_ratio <= 0 | |
| or vntr_to_flanking_ratio != vntr_to_flanking_ratio | |
| or vntr_to_flanking_ratio in (float("inf"), float("-inf")) | |
| ): | |
| raise ConfigurationError( | |
| "Invalid read_simulator config: 'vntr_to_flanking_ratio' " | |
| f"must be a positive finite float, got {raw_vntr_to_flanking_ratio!r}" | |
| ) |
| bed_file = rs_config.get("sample_target_bed") | ||
| if not bed_file: | ||
| raise ConfigurationError( | ||
| "For VNTR downsampling, 'sample_target_bed' (non-VNTR targets) " | ||
| "must be provided in config" | ||
| ) |
There was a problem hiding this comment.
VNTR mode now requires sample_target_bed, but there is no unit test covering the new ConfigurationError path when downsample_mode: "vntr" is selected and sample_target_bed is missing. Adding a test for this would prevent regressions in config validation.
| - Real exome BAMs: `/home/bernt-popp/development/vntyper-analyses/results/screening/Bernt/data/cerkid-exome-chr1/` | ||
| - Simulated BAMs: `/tmp/sim_batch/len_{30..90}/sim_{len}.001.simulated_reads_downsampled.bam` |
There was a problem hiding this comment.
This report includes absolute local filesystem paths (e.g., /home/..., /tmp/...). If this repository/branch is shared beyond the author’s machine, these paths can leak personal usernames or internal directory structure and reduce reproducibility. Consider replacing them with generalized placeholders or relative paths (and, if needed, describing the layout in words).
| - Real exome BAMs: `/home/bernt-popp/development/vntyper-analyses/results/screening/Bernt/data/cerkid-exome-chr1/` | |
| - Simulated BAMs: `/tmp/sim_batch/len_{30..90}/sim_{len}.001.simulated_reads_downsampled.bam` | |
| - Real exome BAMs: local CerKiD exome chr1 BAM directory (machine-specific path omitted) | |
| - Simulated BAMs: local simulation batch output pattern `len_{30..90}/sim_{len}.001.simulated_reads_downsampled.bam` (base directory omitted) |
- Fix BED coordinate off-by-one: convert 1-based samtools region to 0-based BED (start - 1) in downsample_bam() - Validate vntr_to_flanking_ratio > 0 with ConfigurationError - Add test for vntr mode missing sample_target_bed - Add tests for downsample_bam two-pass approach - Remove absolute paths from validation report Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Summary
Fixes three bugs in
downsample_mode: "vntr"that made it produce completely unrealistic coverage profiles:^regionsyntax broken --downsample_bam()usedsamtools view ^chr1:...to extract non-VNTR reads, which silently returns zero reads. All flanking reads were dropped. Replaced with two-pass BED-based approach (-L/-Ufor extraction,-L/-sfor downsampling).Single-step VNTR-only downsampling -- Downsampled only the VNTR region, leaving flanking at 700x+ while reducing VNTR to 150x. Produced inverted VNTR:flanking ratio (0.5x vs real 1.4x). Replaced with two-step:
vntr_to_flanking_ratio)Same seed for both steps -- samtools
-suses hash-based subsampling, so reads surviving step 1's lower fraction always survive step 2 at the same seed. Step 2 now uses seed+1.Validation (1,043 real exomes vs 50 simulations)
*POOR metrics are pre-existing read generation characteristics, not downsampling issues.
Full validation report:
.planning/reports/2026-04-06-vntr-downsampling-validation.mdTest plan
Closes #89
🤖 Generated with Claude Code