Cover the remaining paired-guide options, and stop retaining unmatched pairs - #35
Merged
Conversation
…d pairs Four options on the paired-guide path had no end-to-end test. All four behave correctly today; these pin that down so the next change to the path cannot quietly undo it. Each was checked against a deliberately broken build -- window ignored, --reverse-complement-2 made a no-op, the --pg-min-read comparison shifted by one, the pair filter forced on -- and each caught its mutant. The fixture's second guide was TGCATGCATGCATGCATGCA, which is its own reverse complement, so it matched in either orientation and could not have told a working --reverse-complement-2 from a missing one. Replaced with a non-palindromic guide, with an assertion so it stays that way. Also fixes the accumulation noted in the issue: in secondpair mode the second-read window was recorded even when read 1 matched no guide, under a None key that mageckcount_printpgdict skips. Nothing reads it back, but it retains one entry per distinct unmatched window, so memory grew with the unmapped fraction of the FASTQ -- 5,000 entries against 100 useful ones at a 2% mapping rate. Dropped the per-read umi-start check at the firstpair site too, now that mageckcount_check_extraction_window validates it once up front. Closes #29. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Owner
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. Delightful! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
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.
Closes the remaining scope of #29: the four uncovered paired-guide options, and the
ctab_umi[None]accumulation.Tests
--pg-start-2/--pg-end-2test_wrong_second_read_window_yields_no_pairs--reverse-complement-2, both orientationstest_reverse_complement_2_matches_only_the_orientation_it_is_given--pg-min-read, including the boundarytest_pg_min_read_excludes_pairs_below_the_threshold--pg-pair-onlyomittedtest_all_pairs_are_reported_when_pg_pair_only_is_omittedAll four options behave correctly on
main— these are characterization tests, not bug fixes. That makes them worth nothing unless they can fail, so each was run against a deliberately broken build:[0:20]--reverse-complement-2made a no-optotalr>=args.pg_min_read→>The window test also asserts
count.txtstill fills from read 1 whilepg_count.txtis header-only — that asymmetry is the signature users actually report (#15), and it is what makes the failure so easy to miss.A trap removed from the fixture
The fixture's second guide was
TGCATGCATGCATGCATGCA, which is its own reverse complement. It matched in either orientation, so a--reverse-complement-2test built on it would have passed whether the option worked or not. Replaced with a non-palindromic guide, with an assertion in the fixture so it stays that way.The
ctab_umi[None]accumulationIn
secondpairmode the second-read window was recorded for every read, including those where read 1 matched no guide, under aNonekey.mageckcount_printpgdictskips that key, so output was never affected — but it retains one entry per distinct unmatched window, meaning memory grows with the unmapped fraction of the FASTQ rather than with the library.Measured at a 2% mapping rate (100 matched, 5,000 unmatched):
test_unmatched_reads_do_not_accumulate_a_none_keyasserts what is retained stays bounded by the library. It was written first and confirmed failing.Also dropped the per-read
umi-start/umi-endcheck at thefirstpairextraction site — the same log-flood pattern fixed forsecondpairin #33, now redundant sincemageckcount_check_extraction_windowvalidates once before any FASTQ is opened.43 tests pass.
Closes #29.