Replace Ord instance with helper#24
Open
ninioArtillero wants to merge 1 commit intoseereason:masterfrom
Open
Conversation
177845d to
407f596
Compare
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.
Fixes #22
In
Data.Algorithm.Diff, the internal typeDL(D-path Location) carries the coordinates(poi, poj)of a wave-front node in the Myers edit graph and the edit trace accumulated so far. AnOrdinstance is defined solely to support a single call tomaxinsidepairMaxes(withindstep), where two candidate D-paths competing for the same k-diagonal must be resolved in favour of the furthest-reaching one.This PR removes this unlawful
Ordinstance in favor of afurthestReachinghelper, documented with its call site preconditions, and renamespairMaxestoresolveCandidates.A look into the algorithm
Disclaimer: abuse of notation ahead.
In the Myers algorithm, on any given k-diagonal
k = i - jwhereiandjare coordinates, the furthest-reaching endpoint is the one with the largesti(equivalently, the largestj, sincej = i - k). Theelsebranch (poi x <= poi y) in theOrdinstance definition implements exactly that. The argument is that this is the only relevant comparison when choosing a "better candidate" within this implementation, so thatmax x yeffectively returns whichever has the largerpoi.As used within
sesby callingdstepon single node list,pairMaxesalways pairs exactly two candidates that target the same k-diagonal: one arriving via an F-move from diagonalk - 1, the other via an S-move from diagonalk + 1(cf section 3 lemma 2 in the paper). Afteraddsnakeextends both along their (shared) diagonal, if they end up with the samepoi, they necessarily also have the samepoj(again, because both sit on the same diagonal:poj = poi - k). So whenpoi x == poi ythe instance evaluatespoj x > poj y, resulting always inFalse(which does not matter asmax x ycan equally return either value in this scenario). The reversedpojcomparison can be understood as an ad-hoc tie-breaker that is only exercised in case of equality of both coordinates. Put bluntly, the equal-poibranch in the instance definition is a confusing artifact, and AFAICT not a deliberate encoding of any algorithmic property.Rationale on the proposed change
Changing
poj x > poj ytopoj x >= poj yrestores reflexivity (and lawfulness of the instance) with a one-character edit, nevertheless I propose removing it in favor of a helper becauseOrdinstance reversespojwithin eachpoi-equivalence class, which is not a natural lexicographic ordering on coordinates. Anyone reading the code later must puzzle out why the ordering is inverted in one component.furthestReachingmakes the algorithmic intent obvious.