Skip to content

Commit 2ac9a1f

Browse files
authored
Merge pull request #34 from logannye/rosalind/act1-correctness
fix: real-genome correctness — multi-contig eval, contig-naming guard, IUPAC (Act-1 PR-B)
2 parents b36be4f + 8d926ce commit 2ac9a1f

8 files changed

Lines changed: 258 additions & 17 deletions

File tree

src/genomics/eval/compare.rs

Lines changed: 27 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -48,9 +48,13 @@ impl ComparisonReport {
4848
}
4949
}
5050

51-
/// Compare two VCF callsets against a reference sequence, optionally masked by BED regions.
51+
/// Compare two VCF callsets against a per-contig reference map, optionally masked
52+
/// by BED regions. Each variant is normalized against the sequence of ITS OWN
53+
/// contig (`references[v.chrom]`) — loading only the first FASTA record and
54+
/// applying it to every contig silently miscompares (or crashes) on any
55+
/// multi-contig benchmark, e.g. a real GIAB run.
5256
pub fn compare_callsets(
53-
reference: &[u8],
57+
references: &BTreeMap<String, Vec<u8>>,
5458
calls: &[VcfVariant],
5559
truth: &[VcfVariant],
5660
bed: Option<&BedIndex>,
@@ -64,15 +68,15 @@ pub fn compare_callsets(
6468
continue;
6569
}
6670
}
67-
calls_set.insert(normalize_variant(reference, v)?);
71+
calls_set.insert(normalize_variant(reference_for(references, v)?, v)?);
6872
}
6973
for v in truth {
7074
if let Some(bed) = bed {
7175
if !bed.contains(&v.chrom, v.pos0) {
7276
continue;
7377
}
7478
}
75-
truth_set.insert(normalize_variant(reference, v)?);
79+
truth_set.insert(normalize_variant(reference_for(references, v)?, v)?);
7680
}
7781

7882
let mut tp = 0usize;
@@ -113,6 +117,25 @@ pub fn compare_callsets(
113117
})
114118
}
115119

120+
/// The reference sequence for a variant's contig, or a clear error naming the
121+
/// missing contig and the available ones (the common contig-naming mismatch).
122+
fn reference_for<'a>(
123+
references: &'a BTreeMap<String, Vec<u8>>,
124+
v: &VcfVariant,
125+
) -> Result<&'a [u8], anyhow::Error> {
126+
references.get(&v.chrom).map(Vec::as_slice).ok_or_else(|| {
127+
let available: Vec<&str> = references.keys().map(String::as_str).collect();
128+
anyhow::anyhow!(
129+
"variant on contig '{}' (pos {}) has no matching sequence in the reference FASTA — \
130+
check the contig naming scheme (e.g. UCSC 'chr1' vs Ensembl '1'). \
131+
Reference contigs: [{}]",
132+
v.chrom,
133+
v.pos0 + 1,
134+
available.join(", ")
135+
)
136+
})
137+
}
138+
116139
fn variant_type(v: &NormalizedVariant) -> VariantType {
117140
if v.reference.len() == 1 && v.alternate.len() == 1 {
118141
VariantType::Snv

src/genomics/eval/normalize.rs

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,8 +6,13 @@ use super::VcfVariant;
66
/// Errors that can occur during variant normalization.
77
pub enum NormalizeError {
88
/// The variant POS is outside the provided reference slice.
9-
#[error("variant position {pos0} out of bounds for reference length {reference_len}")]
9+
#[error(
10+
"variant on contig '{chrom}' position {pos0} is out of bounds for that contig's \
11+
reference length {reference_len}"
12+
)]
1013
OutOfBounds {
14+
/// Contig name (so a multi-contig miscompare names the right contig).
15+
chrom: String,
1116
/// 0-based position.
1217
pos0: u32,
1318
/// Reference length.
@@ -55,6 +60,7 @@ pub fn normalize_variant(
5560

5661
if pos0 as usize >= reference.len() {
5762
return Err(NormalizeError::OutOfBounds {
63+
chrom: v.chrom.clone(),
5864
pos0,
5965
reference_len: reference.len(),
6066
});

src/genomics/fm_index.rs

Lines changed: 32 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -426,10 +426,23 @@ fn sanitize_reference(reference: &[u8]) -> Result<Vec<u8>, FMIndexError> {
426426
clean.push(uppercase);
427427
}
428428
None => {
429-
return Err(FMIndexError::UnsupportedCharacter {
430-
ch: ch as char,
431-
position: idx,
432-
});
429+
// IUPAC ambiguity codes (R,Y,S,W,K,M,B,D,H,V) appear in real
430+
// assemblies (GRCh38's primary assembly, many bacterial/viral
431+
// references). Map them to N — matching bwa/bowtie — so the index
432+
// step ingests a stock reference instead of aborting; the N-mask
433+
// carries the ambiguity, and these positions never match an
434+
// A/C/G/T query. Genuinely non-sequence bytes still error loudly.
435+
if matches!(
436+
ch.to_ascii_uppercase(),
437+
b'R' | b'Y' | b'S' | b'W' | b'K' | b'M' | b'B' | b'D' | b'H' | b'V'
438+
) {
439+
clean.push(b'N');
440+
} else {
441+
return Err(FMIndexError::UnsupportedCharacter {
442+
ch: ch as char,
443+
position: idx,
444+
});
445+
}
433446
}
434447
}
435448
}
@@ -561,6 +574,21 @@ mod tests {
561574
bwt[..bounded].iter().filter(|&&ch| ch == base).count() as u32
562575
}
563576

577+
#[test]
578+
fn sanitize_maps_iupac_ambiguity_codes_to_n() {
579+
// Real references carry IUPAC degeneracy codes; the index must ingest
580+
// them (mapped to N) rather than aborting. Genuinely invalid bytes still
581+
// error loudly.
582+
let clean = sanitize_reference(b"ACGTRYSWKMryswkmBDHV").unwrap();
583+
assert_eq!(&clean, b"ACGTNNNNNNNNNNNNNNNN");
584+
// A stock-reference-shaped sequence with ambiguity codes builds an index.
585+
let index = BlockedFMIndex::build(b"ACGTRYSWKMACGTACGTAC", 4)
586+
.expect("index build must succeed over IUPAC codes");
587+
let _ = index;
588+
// A non-sequence byte is still rejected.
589+
assert!(sanitize_reference(b"ACGT@CGT").is_err());
590+
}
591+
564592
#[test]
565593
fn sa_at_recovers_reference_position() {
566594
let reference = b"ACGTACGT";

src/io/bam.rs

Lines changed: 46 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,10 +84,12 @@ pub(crate) fn validate_contig_lengths(
8484
header: &bam::HeaderView,
8585
contigs: &ContigSet,
8686
) -> Result<(), CoreError> {
87+
let mut matched = 0usize;
8788
for tid in 0..header.target_count() {
8889
let name = std::str::from_utf8(header.tid2name(tid))
8990
.map_err(|_| CoreError::MalformedRecord("BAM reference name is not UTF-8".into()))?;
9091
if let Some(c) = contigs.by_name(name) {
92+
matched += 1;
9193
let header_len = header.target_len(tid);
9294
if header_len != Some(c.length as u64) {
9395
return Err(CoreError::MalformedRecord(format!(
@@ -101,6 +103,27 @@ pub(crate) fn validate_contig_lengths(
101103
}
102104
}
103105
}
106+
// If the BAM names a reference but NONE of its @SQ contigs resolve into the
107+
// index, the two use incompatible naming schemes (the classic UCSC 'chr1' vs
108+
// Ensembl '1' / GRCh38-vs-hg38 mismatch). Left unchecked, every record is
109+
// silently dropped and the run writes an empty VCF with exit 0 — the worst
110+
// kind of failure. Refuse up front with an actionable message instead.
111+
if header.target_count() > 0 && matched == 0 {
112+
let bam_names: Vec<String> = (0..header.target_count())
113+
.filter_map(|tid| {
114+
std::str::from_utf8(header.tid2name(tid))
115+
.ok()
116+
.map(str::to_string)
117+
})
118+
.take(3)
119+
.collect();
120+
return Err(CoreError::MalformedRecord(format!(
121+
"no BAM @SQ contig name matches the index — the alignments and the index use \
122+
incompatible naming schemes (e.g. UCSC 'chr1' vs Ensembl '1'). BAM names start \
123+
with [{}]; check the reference/assembly used for alignment.",
124+
bam_names.join(", ")
125+
)));
126+
}
104127
Ok(())
105128
}
106129

@@ -305,6 +328,29 @@ mod tests {
305328
std::fs::remove_dir_all(&dir).ok();
306329
}
307330

331+
#[test]
332+
fn streaming_source_rejects_disjoint_contig_naming() {
333+
// BAM @SQ uses Ensembl '1'; the index uses UCSC 'chr1'. No name resolves,
334+
// so opening must refuse up front rather than silently dropping every read
335+
// into an empty VCF.
336+
let bam_path = tmp("naming-mismatch");
337+
let header = test_header(&[("1", 1000)]); // Ensembl-style
338+
{
339+
let mut writer = bam::Writer::from_path(&bam_path, &header, bam::Format::Bam).unwrap();
340+
let hv = writer.header().clone();
341+
push_record(&mut writer, &hv, 0, 10, b"ACGT");
342+
}
343+
let contigs = one_contig(); // UCSC-style 'chr1'
344+
let err = StreamingBamSource::new(&bam_path, &contigs)
345+
.expect_err("disjoint naming must be rejected");
346+
let msg = err.to_string();
347+
assert!(
348+
msg.contains("no BAM @SQ contig name matches the index"),
349+
"unexpected error: {msg}"
350+
);
351+
std::fs::remove_file(&bam_path).ok();
352+
}
353+
308354
#[test]
309355
fn streaming_source_rejects_out_of_order() {
310356
let dir = std::env::temp_dir().join(format!(

src/main.rs

Lines changed: 25 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -484,9 +484,8 @@ fn run_eval(
484484
truth_path: PathBuf,
485485
regions_path: Option<PathBuf>,
486486
) -> Result<()> {
487-
let fasta = read_fasta(&reference_path)
487+
let references = read_fasta_map(&reference_path)
488488
.with_context(|| format!("failed to read reference from {}", reference_path.display()))?;
489-
let reference = fasta.sequence;
490489

491490
let calls_txt = std::fs::read_to_string(&calls_path)
492491
.with_context(|| format!("failed to read calls VCF {}", calls_path.display()))?;
@@ -506,7 +505,7 @@ fn run_eval(
506505
None
507506
};
508507

509-
let report = compare_callsets(&reference, &calls, &truth, bed.as_ref())?;
508+
let report = compare_callsets(&references, &calls, &truth, bed.as_ref())?;
510509
println!("truth_total={}", report.total_truth);
511510
println!("calls_total={}", report.total_calls);
512511
println!("tp={}", report.true_positive);
@@ -1881,6 +1880,29 @@ fn read_fasta(path: &PathBuf) -> Result<FastaRecord> {
18811880
Ok(first)
18821881
}
18831882

1883+
/// Read ALL FASTA records into a contig-name → sequence map. Used by `eval-*`,
1884+
/// which must normalize each variant against its OWN contig — loading only the
1885+
/// first record (the single-contig `read_fasta` policy) silently miscompares or
1886+
/// crashes on any multi-contig benchmark.
1887+
fn read_fasta_map(path: &PathBuf) -> Result<std::collections::BTreeMap<String, Vec<u8>>> {
1888+
let reader = open_input(path).with_context(|| format!("failed to open {}", path.display()))?;
1889+
let mut map = std::collections::BTreeMap::new();
1890+
for rec in FastaReader::new(reader) {
1891+
let rec = rec.with_context(|| format!("failed to parse FASTA {}", path.display()))?;
1892+
if map.insert(rec.name.clone(), rec.sequence).is_some() {
1893+
bail!(
1894+
"FASTA {} has a duplicate contig name '{}'",
1895+
path.display(),
1896+
rec.name
1897+
);
1898+
}
1899+
}
1900+
if map.is_empty() {
1901+
bail!("FASTA file {} is missing a record", path.display());
1902+
}
1903+
Ok(map)
1904+
}
1905+
18841906
/// Read a FASTQ file (plain or gzip; `-` = stdin) into a vector of records.
18851907
/// The streaming parser lives in `io::fastq`.
18861908
fn read_fastq(path: &PathBuf) -> Result<Vec<FastqRecord>> {

tests/clinical_eval_gates.rs

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,10 @@
11
use rosalind::genomics::{compare_callsets, read_vcf_variants, BedIndex};
2+
use std::collections::BTreeMap;
23

34
#[test]
45
fn eval_compare_smoke_and_thresholds() {
5-
// Small deterministic reference.
6-
let reference = vec![b'A'; 100];
6+
// Small deterministic reference (single contig 'chr1').
7+
let references = BTreeMap::from([("chr1".to_string(), vec![b'A'; 100])]);
78

89
let calls_vcf = "\
910
##fileformat=VCFv4.3
@@ -23,7 +24,7 @@ chr1\t31\t.\tA\tT\t50\tPASS\t.
2324

2425
// Mask to only evaluate positions 0..40.
2526
let bed = BedIndex::from_str("chr1\t0\t40\n").unwrap();
26-
let rep = compare_callsets(&reference, &calls, &truth, Some(&bed)).unwrap();
27+
let rep = compare_callsets(&references, &calls, &truth, Some(&bed)).unwrap();
2728

2829
// TP: pos11 A>C
2930
// FP: pos21 A>G

tests/eval_multicontig.rs

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,113 @@
1+
//! `eval-germline` must normalize each variant against its OWN contig. Loading
2+
//! only the first FASTA record silently miscompared (or crashed with an opaque
3+
//! out-of-bounds error) on any multi-contig benchmark — the headline truth-
4+
//! comparison surface, and exactly what a real GIAB run looks like.
5+
6+
use std::path::PathBuf;
7+
use std::process::Command;
8+
use std::sync::atomic::{AtomicU64, Ordering};
9+
10+
fn bin() -> &'static str {
11+
env!("CARGO_BIN_EXE_rosalind")
12+
}
13+
14+
fn unique_dir(prefix: &str) -> PathBuf {
15+
static COUNTER: AtomicU64 = AtomicU64::new(0);
16+
let n = COUNTER.fetch_add(1, Ordering::Relaxed);
17+
let nanos = std::time::SystemTime::now()
18+
.duration_since(std::time::UNIX_EPOCH)
19+
.unwrap()
20+
.as_nanos();
21+
let d = std::env::temp_dir().join(format!("{prefix}-{nanos}-{n}"));
22+
std::fs::create_dir_all(&d).unwrap();
23+
d
24+
}
25+
26+
// chr1 = 20 bp, chr2 = 40 bp.
27+
const REFERENCE: &str =
28+
">chr1\nACGTACGTACGTACGTACGT\n>chr2\nACGTACGTACGTACGTACGTACGTACGTACGTACGTACGT\n";
29+
30+
#[test]
31+
fn eval_germline_compares_each_variant_against_its_own_contig() {
32+
let dir = unique_dir("rosalind-eval-mc");
33+
let fa = dir.join("ref.fa");
34+
std::fs::write(&fa, REFERENCE).unwrap();
35+
36+
// A chr1 SNV (pos 5) and a chr2 SNV at pos 30 — past chr1's length (20), so
37+
// the old first-contig-only path crashed out-of-bounds on the chr2 variant.
38+
let header = "##fileformat=VCFv4.3\n#CHROM\tPOS\tID\tREF\tALT\tQUAL\tFILTER\tINFO\n";
39+
let calls = dir.join("calls.vcf");
40+
std::fs::write(
41+
&calls,
42+
format!("{header}chr1\t5\t.\tA\tC\t50\tPASS\t.\nchr2\t30\t.\tG\tT\t50\tPASS\t.\n"),
43+
)
44+
.unwrap();
45+
let truth = dir.join("truth.vcf");
46+
std::fs::write(
47+
&truth,
48+
format!("{header}chr1\t5\t.\tA\tC\t50\tPASS\t.\nchr2\t30\t.\tG\tT\t50\tPASS\t.\n"),
49+
)
50+
.unwrap();
51+
52+
let out = Command::new(bin())
53+
.args(["eval-germline", "--reference"])
54+
.arg(&fa)
55+
.arg("--calls")
56+
.arg(&calls)
57+
.arg("--truth")
58+
.arg(&truth)
59+
.output()
60+
.unwrap();
61+
assert!(
62+
out.status.success(),
63+
"multi-contig eval must succeed: {}",
64+
String::from_utf8_lossy(&out.stderr)
65+
);
66+
let stdout = String::from_utf8_lossy(&out.stdout);
67+
// Both contigs' variants match truth → 2 TP, 0 FP, 0 FN.
68+
assert!(
69+
stdout.contains("tp=2"),
70+
"expected 2 true positives: {stdout}"
71+
);
72+
assert!(
73+
stdout.contains("fp=0"),
74+
"expected 0 false positives: {stdout}"
75+
);
76+
assert!(
77+
stdout.contains("fn=0"),
78+
"expected 0 false negatives: {stdout}"
79+
);
80+
std::fs::remove_dir_all(&dir).ok();
81+
}
82+
83+
#[test]
84+
fn eval_germline_errors_clearly_on_a_contig_naming_mismatch() {
85+
let dir = unique_dir("rosalind-eval-naming");
86+
let fa = dir.join("ref.fa");
87+
std::fs::write(&fa, REFERENCE).unwrap();
88+
89+
// A variant on a contig the FASTA does not contain (Ensembl '1' vs UCSC
90+
// 'chr1') must error clearly, not silently miscompare.
91+
let header = "##fileformat=VCFv4.3\n#CHROM\tPOS\tID\tREF\tALT\tQUAL\tFILTER\tINFO\n";
92+
let calls = dir.join("calls.vcf");
93+
std::fs::write(&calls, format!("{header}1\t5\t.\tA\tC\t50\tPASS\t.\n")).unwrap();
94+
let truth = dir.join("truth.vcf");
95+
std::fs::write(&truth, header).unwrap();
96+
97+
let out = Command::new(bin())
98+
.args(["eval-germline", "--reference"])
99+
.arg(&fa)
100+
.arg("--calls")
101+
.arg(&calls)
102+
.arg("--truth")
103+
.arg(&truth)
104+
.output()
105+
.unwrap();
106+
assert!(!out.status.success(), "naming mismatch must fail");
107+
let stderr = String::from_utf8_lossy(&out.stderr);
108+
assert!(
109+
stderr.contains("naming scheme") || stderr.contains("no matching sequence"),
110+
"expected a clear naming-mismatch error: {stderr}"
111+
);
112+
std::fs::remove_dir_all(&dir).ok();
113+
}

0 commit comments

Comments
 (0)