pipe: Make frameshift repair on by default - #311
Conversation
Frameshift repair substantially improves window recovery on error-prone long reads, so it should not require an opt-in flag. Also fixes lyrebird, which never wired up repair_frameshifts/max_frameshift_repair_divergence at all and so always ran with repair disabled regardless of CLI args. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 55d509e429
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if operation[1] == '/': | ||
| frameshifts.append((position - 1, 'deletion')) | ||
| position -= step |
There was a problem hiding this comment.
When qstart > qend, after consuming a BTOP match run, position is the next query coordinate in the reverse traversal. Using position - 1 as the insertion index places the missing base before that coordinate in the forward string—after it in the reverse traversal—whereas it must be inserted after that coordinate (index position) to sit at the frameshift boundary. Consequently, reverse-strand / repairs restore the length but put the N one nucleotide too far away, changing the affected codon and potentially the extracted window; the deletion position must account for step.
Useful? React with 👍 / 👎.
| num_resolved += resolve_ambiguous_windows( | ||
| window_sequences, | ||
| max_divergence=max_frameshift_repair_divergence) |
There was a problem hiding this comment.
When an input FASTA/FASTQ already contains an N in a window, this passes it to resolve_ambiguous_windows along with frameshift-repaired windows even if DIAMOND reported no frameshift for that read. Because the resolver has no repair provenance, it imputes every pre-existing N from another read and silently changes user-supplied ambiguous sequence into a concrete OTU; this also occurs in programmatic or nucleotide-package paths where no DIAMOND repair happened. Track which sequences actually received deletion repairs and resolve only those.
Useful? React with 👍 / 👎.
…g pre-existing Ns Addresses Codex review comments on PR #311: - walk_btop() placed the inserted ambiguous base one nucleotide too early for '/' (deletion) frameshifts on the reverse strand, corrupting the codon at the repair site. Verified against randomized forward/reverse strand deletion+insertion cases translated back to amino acids. - resolve_ambiguous_windows() could not tell a repair-inserted 'N' apart from an 'N' already present in the raw read, so it would impute bases into reads with no frameshift at all. diamond_spkg_searcher.py now records which hits actually had a deletion repaired, and pipe.py threads that set through as resolve_ambiguous_windows(repaired_names=...) so only genuine repairs are resolved. - Updates test/test_pipe.py expectations for 3 tests where frameshift repair (now on by default) correctly fixes a real indel in the fixture read, and adds --no-repair-frameshifts to --no-diamond-prefilter tests that would otherwise fail validation now that repair defaults on. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
It was set to 4, but every divergence-dependent test (and their comments, e.g. "more than the default maximum of 2") was written assuming 2, causing test_donor_beyond_max_divergence_not_used to fail. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Revert the previous commit's change to 2; 4 is the intended default, so update test_donor_beyond_max_divergence_not_used's donor and comment to differ at 5 positions instead, keeping it beyond the (now correct) cap.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7aecf6e112
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| resolutions[window] = ''.join( | ||
| donor_base if window_base == ambiguous_char else window_base | ||
| for (window_base, donor_base) in zip(window, best_donor)) |
There was a problem hiding this comment.
When a read contains both a repaired deletion and a pre-existing N in its extracted window, this replaces every N from the donor even though only one was introduced by repair. Fresh evidence in the current code is that repaired_deletion_qseqids records only the whole read name, without the inserted position; it also aliases paired ends and same-named reads across samples. Consequently, the attempted provenance fix still concretizes user-supplied ambiguity whenever any deletion is repaired on an admitted name, so the repair positions or a distinct sentinel must be carried through to resolution.
Useful? React with 👍 / 👎.
Only pipe.py calls run_diamond, and it always passes the argument explicitly, so the default served no backwards-compatibility purpose.
Same rationale as run_diamond: the only caller (run_diamond) always passes it explicitly, so the default served no purpose. Update the one test that relied on it to pass the argument explicitly.
…SHIFT_REPAIR_DIVERGENCE Matches the name already used for it in pipe.py and main.py's CLI help, and drops the redundant alias now that the names agree.
1 participant
Summary
--repair-frameshiftswith--no-repair-frameshiftsto disable it.lyrebird pipe, which never passedrepair_frameshifts/max_frameshift_repair_divergencethrough toSearchPipe.run()at all, so it silently ran with repair disabled regardless of CLI args.Test plan
test/test_frameshift_repair.pyandtest/test_diamond_spkg_searcher.pypass (one pre-existing unrelated failure confirmed present before this change too)btop(frameshift repair enabled) and that--no-repair-frameshiftsremoves it--no-diamond-prefilterand--hmmsearch-package-assignmentstill raise the expected validation errors unless--no-repair-frameshiftsis also passed🤖 Generated with Claude Code