Repository navigation
Address review findings on sub-k seed labels (#691) and map-only mode (#692) - #694
Merged
Merged
Conversation
- Reject --align-map-only together with --map, --count-kmers or --query-presence, which take the k-mer mapping path and would silently ignore it. - Check the coordinates of sub-k seeds at sequence starts on a BASIC graph with a coordinate annotation: every reported coordinate is non-negative and points at the aligned sequence in its label. Co-Authored-By: Claude Opus 5.5 <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.
Follow-up to the cubic review comments on #691 and #692, which were merged before the comments were processed.
#692,
--align-map-onlywith--map/--count-kmers/--query-presence. Those flags take the k-mer mapping path, so--align-map-onlywas silently ignored. The combination is now rejected inConfig, with an error naming the flags.#691, coordinates in the sub-k seed regression test.
SubkSeedLabelsnow also runs the BASIC case with a coordinate annotation. For every alignment label it checks three things: there is one coordinate set per label, every coordinate is non-negative, and each coordinate points at the aligned sequence in that label. That includes the query that matches only a source dummy k-mer.#691, negative coordinates for source dummies (no code change). The
$…$source-dummy coordinates arec - num_dummy, which is negative at a sequence start. They are stored asint64_t(the subtraction wraps in unsigned arithmetic, and the conversion back toint64_tgives the intended negative value). They do not reach the output:$, so its offset is at leastnum_dummy.filter_seedsadds the offset back, so the seed's coordinate is at leastc.Checks:
column_coordannotation, I aligned 252 reads that are bare 20/25/30 bp contig-start prefixes on both strands. No reported coordinate wrapped, at seed length 19 (246 aligned) or 15 (252 aligned).#692, two rows per header with
--fwd-and-reverse(kept). With that flag the FASTA parser yields each record and its reverse complement as two queries with the same header. Plainalignprints a row for each, and map-only now does the same. Dropping one in map-only mode would make the two modes disagree on the row contract.#691, test for the >16-continuation cap (not added). Exceeding the cap needs a source dummy with at least 3
$and more than 16 distinct continuations. A test for that would mostly assert on graph construction, so I did not add one.🤖 Generated with Claude Code
Summary by cubic
Addresses review comments on the merged PRs #691 and #692.
Behavior change
--align-map-onlyis now rejected when combined with--map,--count-kmers, or--query-presence, since those flags take the k-mer mapping path and previously caused--align-map-onlyto be silently ignored.Bug fixes
SubkSeedLabelstest to run the BASIC case with a coordinate annotation and asserts that every reported coordinate is non-negative and points at the aligned sequence within its label.Written for commit 7e87878. Summary will update on new commits.