Add substitution, insertion, and deletion cost functions - #81
Open
belambert wants to merge 1 commit into
Open
Conversation
Three optional cost functions replace the hardcoded unit costs. Each defaults to the previous behavior, so the default path is unchanged -- verified against main over all sequence pairs up to length four on a two-letter alphabet plus randomized longer pairs, comparing distance, matches, opcodes, and the whole SequenceMatcher surface. test and cost stay independent. test decides equality, which drives the opcode label and the match count; the cost functions only decide the price. substitution_cost is consulted for every aligned pair, matching ones included, which is what lets a near-match count as a match while still costing something. The boundary rows become cumulative sums of the gap costs rather than i and j, which is the part the old n and m special cases were hiding. The seq1 == seq2 shortcut is now guarded on there being no custom substitution cost, since such a cost may charge for aligning an element with itself. Everything after the two sequences is keyword-only, so a stray third positional argument raises TypeError rather than being silently read as test -- it used to be the action function.
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.
Adds
substitution_cost,insertion_cost, anddeletion_costtoedit_distance(),edit_distance_backpointer(), andSequenceMatcher. Each defaults to the previous behavior, so the default path is unchanged.testand cost stay independent:testdecides equality, which drives the opcode label and the match count, while the cost functions only decide the price.substitution_costis consulted for every aligned pair, matching ones included — that's what lets a near-match count as a match while still costing something, and it means a custom function has to handle both branches.The boundary rows are now cumulative sums of the gap costs rather than
iandj, which is what the oldm == 0/n == 0special cases were papering over. Theseq1 == seq2shortcut is guarded on there being no custom substitution cost, since such a cost may charge for aligning an element with itself.Everything after the two sequences is keyword-only. That matters here: the third positional slot used to be
action_function, so without this a leftoveredit_distance(a, b, some_action_fn)would silently be read astestinstead of failing.On
highest_match_actionI need to correct something I claimed when removing it. Setting the mismatch penalty to
ins + delrecovers the same opcodes and the same match count — but the distance differs (6 vs 4) on the old test's input.highest_match_actionaccumulated unit costs while choosing by match count; a cost function uses the same numbers for both, so the distance is necessarily in the new units.test_substitution_cost_reproduces_highest_match_alignmentasserts the verified values and explains why. My earlier suggestion of a penalty of 3 was also wrong for opcode-identity — aboveins + delyou get a different, LCS-style alignment, which is covered by its own test.Testing
13 new unit tests and 3 new property tests; 35 total, up from 22.
SequenceMatcherreaching the cache through all three ofdistance(),get_opcodes(), andmatches().make cipasses, and all 35 tests pass on 3.10, 3.11, 3.12, 3.13, and 3.14 individually.sequence_matcher.pyis at 100% coverage; the only uncovered lines left inedit_distance.pyare pre-existing (main(), the__main__guard, and one defensiveraise).To confirm the default path really is untouched after rewriting both DP loops, I diffed against
mainover 4,961 cases — exhaustive to length 4 on a 2-letter alphabet, 3,000 random integer pairs, 500 under a customtest, and 500 through the fullSequenceMatchersurface includingratio()andget_matching_blocks(). Zero differences.