Key CRISPR guides on (anchor, protospacer) via disambiguate-on-collision#248
Open
Lioscro wants to merge 4 commits into
Open
Key CRISPR guides on (anchor, protospacer) via disambiguate-on-collision#248Lioscro wants to merge 4 commits into
Lioscro wants to merge 4 commits into
Conversation
CrisprMapper identified guides by protospacer alone and fed the raw protospacer column to SeqHash::new, which rejects duplicate parents, crashing on libraries where a protospacer appears under two anchor scaffolds (e.g. FLEXLIBPOOL009: "parent at index 1 is duplicate of parent at index 0"). Key guide identity on (anchor, protospacer) via disambiguate-on-collision: - dedup protospacers before hashing (SeqHash never sees a duplicate) - proto_guides maps each protospacer parent idx to its (anchor, row) guides - query ignores the anchor when a protospacer is unique (backward-compatible) and uses the matched anchor to disambiguate when shared; unmapped if the matched anchor carries none of that protospacer's guides - feature_idx is now the TSV row index (contiguous 0..N, aligned with features.tsv / record_stream) - unresolvable identical (anchor, protospacer) rows bail! naming both guides - statistics report the true guide count; info! logs shared protospacers Adds 7 unit tests (collision resolution asserting exact per-anchor row index, unique-protospacer backward-compat, wrong-anchor->None, dup-pair error, statistics/row-count, exact-build path, real-library guard) and two fixtures. Mutation check confirms the collision tests catch the regression. Confined to crispr.rs; downstream is index-agnostic. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Update crates/cyto-map/CLAUDE.md for the Phase 1 keying change: - crispr.rs bullet now describes (anchor, protospacer) disambiguate-on- collision keying (protospacer dedup, feature_idx = TSV row index, load-time error on a duplicate (anchor, protospacer) pair) - fix stale Mapper::query signature: Option<usize> -> Option<FeatureMatch> Integration-validated on real data (not in repo): the exact bug-report `cyto detect crispr` on FLEXLIBPOOL009 now exits 0 with a geometry string (was: "parent at index 1 is duplicate of parent at index 0"); features.tsv has 278 rows with all 12 shared-protospacer guides; and the standard library's features.tsv is byte-identical (md5 unchanged) to main. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Address Step 6 integration-review suggestion: the anchor-constant block header implied all three test anchors are from the real library. Anchors A and B are the real library anchors; C is a synthetic 30bp anchor used for the wrong-anchor -> None path. Comment-only; no behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request updates the CRISPR mapper to support disambiguating guides when a protospacer is shared across multiple anchors. It introduces a new proto_guides mapping, rejects duplicate anchor-protospacer pairs at load time, and updates the query logic to resolve collisions based on the matched anchor. The review feedback suggests destructuring the CrisprRecord during deserialization to avoid unnecessary string clones, which is a valuable performance improvement.
…om_file Address Gemini PR review: destructure CrisprRecord into owned fields so the anchor-miss path moves instead of clones, and the protospacer-miss path clones once (HashMap key) then moves into proto_unique instead of cloning twice. Load-time only; behavior unchanged. Tests green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Problem
CrisprMapperidentified each guide by its protospacer alone and fed the raw (un-deduplicated) protospacer column toseqhash::SeqHash::new, which rejects duplicate parents. On libraries where the same protospacer is deployed on both anchor scaffolds, the build aborts with:This blocks
cyto detect crispr,cyto map crispr, and the crispr workflow on such libraries (e.g.FLEXLIBPOOL009: 278 guides, 2 anchors, 6 protospacers shared across the two anchors).Fix
Key guide identity on the
(anchor, protospacer)pair via disambiguate-on-collision:SeqHashnever sees a duplicate parent, so the crash is structurally impossible.None).feature_idxis now the 0-based TSV row index (contiguous0..N, aligned in order withnames/record_stream()/metadata/features.tsv).seqhashmessage).statistics()reports the true guide count; aninfo!line logs when protospacers are shared.Change is confined to
crates/cyto-map/src/mapper/crispr.rs(plus two test fixtures and aCLAUDE.mddoc update). The downstream pipeline (processor.rs→cyto-ibu-count→ workflow) is feature-index-agnostic and unchanged.Scope: previous behavior explicitly retained (decision deferred)
This change is deliberately minimal. The previous mapping behavior is explicitly retained: a protospacer is matched regardless of anchor for every guide whose protospacer is unique (the common case). The anchor is consulted only to break ties among the rare protospacers shared across anchors — the minimum needed to fix the build crash without changing mapping outcomes for any library that builds today.
This PR does not make the anchor a mandatory part of guide identity (i.e. strict
(anchor, protospacer)pairing enforced on every read). That is a broader decision — it would change mapping rates for all libraries — and is deferred until we decide whether the anchor should also be considered as part of guide identity generally.Testing
cargo test -p cyto-map: 77/77 (7 new unit tests covering collision resolution with exact per-anchor row index, unique-protospacer backward-compat, wrong-anchor→None, dup-pair error, statistics/row-count,exact=truebuild, real-library guard).clippyclean (no new warnings),fmtclean.cyto detect crispronFLEXLIBPOOL009now exits 0, emits geometry[barcode][umi:12] | [:2][probe][anchor][protospacer], and logs6 protospacer(s) shared across anchors; disambiguating by anchor— noduplicateerror.FLEXLIBPOOL009features.tsv= 278 rows, all 12 shared-protospacer guides present as distinct rows.data/libraries/crispr_guides.tsvfeatures.tsvis byte-identical betweenmainand this branch (6066 rows, matching md5).Note
cargo test --workspacemay show acyto-download::run_uses_serverflake (localhost HTTP port contention); it passes in isolation and that crate is untouched by this PR.🤖 Generated with Claude Code