fix: score each sequence independently in compute_pwm - #38
Merged
Conversation
compute_pwm() capped the motif-scan window to nchar(sequences[1]) for the whole batch, so a sequence longer than the first was only scanned over its first few positions and could miss its real motif hit - making a sequence's score depend on its batch companions. Use max(nchar(sequences)) for both the scan range and the flat spatial bin; the C++ scan already clamps per-sequence to each sequence's own end, so every sequence is now scored on its full length. Equal-length batches (regression / screen_kmers) are unchanged since max == first. Claude-Session: https://claude.ai/code/session_01PK3qefBGDoBwd9w5226FEq
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
Reported by Tamar:
compute_pwm()gives a sequence a different score depending on the other sequences in the batch when sequences have different lengths.Root cause
The motif-scan window was capped to
nchar(sequences[1])for the whole batch (passed to the C++spat_max, and the flat spatial bin was sized the same way). A sequence longer than the first was therefore only scanned over its firstnchar(sequences[1])starting positions, missing motif hits further along. A shorter first sequence (8 bp) truncated the 42 bp Oct4 sequence to its first 8 positions.The C++ core (shared with misha's
DnaPSSM) already clamps the scan per sequence toend - motif_len; the cap was purely an artifact of the R wrapper deriving one window fromsequences[1].Fix
Use
max(nchar(sequences))for both the scan range and the flat spatial bin. Each sequence is then scanned to its own end, independently of its batch companions. Equal-length batches (the regression /screen_kmerspath) are byte-identical sincemax == first.The sibling functions (
compute_local_pwm,screen_local_pwm,mask_sequences_by_pwm) carry the samenchar(sequences[1])line but pass motif-length windows to the scan, so the range is inert there - left unchanged.Test
Added a regression test in
test-compute_pwm.Rasserting a sequence's score is batch-position-independent (bothmaxandlogSumExp). Fails before the fix, passes after; existing PWM tests unchanged.https://claude.ai/code/session_01PK3qefBGDoBwd9w5226FEq