fix: error on unequal-length sequences in calc_seq_pwm / extract_pwm - #39
Merged
Conversation
calc_seq_pwm() (and extract_pwm(), which wraps it) built a single rectangular one-hot matrix via do.call(rbind, ...). With sequences of different lengths rbind recycled the shorter rows up to the longest length, so the shorter sequences got wrong, batch-dependent scores - surfaced only as an easy-to-miss rbind warning. Fail loudly with a clear message pointing to compute_pwm() (which handles variable lengths correctly) instead of returning garbage. 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
Follow-up to the
compute_pwmvariable-length fix (#38). Auditing the rest of the PWM family for the same class of bug turned up a second one in the MotifDB scoring path.calc_seq_pwm()(andextract_pwm(), which wraps it) build a single rectangular one-hot matrix viado.call(rbind, ...)inseqs_to_onehot. With sequences of different lengths,rbindrecycles the shorter rows up to the longest length, so the shorter sequences get wrong, batch-dependent scores - and it surfaces only as an easy-to-missrbindwarning ("number of columns of result is not a multiple of vector length"), not an error:Fix
These functions are inherently equal-length (one rectangular matrix multiply against all motifs), and
compute_pwm()already handles variable lengths correctly. So fail loudly instead of returning garbage:Mirrors the existing guard in
compute_local_pwm()(which already errors whenreturn_list = FALSEand lengths differ).Not changed
compute_pwm(fixed in #38),compute_local_pwm,screen_local_pwm,mask_sequences_by_pwm, andgextract.local_pwmwere all verified to be batch-independent on variable-length input. The misha package was also audited and is immune to this bug class (its scan range is always a per-PSSM constant clamped to each target's own length).Test
Added a test asserting
calc_seq_pwmerrors on unequal lengths and still works on equal lengths.https://claude.ai/code/session_01PK3qefBGDoBwd9w5226FEq