-
-
Notifications
You must be signed in to change notification settings - Fork 133
feat(web): implement SearchQuotientCluster merging 🚂 #15516
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
jahorton
wants to merge
1
commit into
change/web/add-same-edge-detection
Choose a base branch
from
feat/web/cluster-merging
base: change/web/add-same-edge-detection
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+307
−14
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -12,6 +12,7 @@ import { LexicalModelTypes } from '@keymanapp/common-types'; | |||||
|
|
||||||
| import { SearchNode, SearchResult } from './distance-modeler.js'; | ||||||
| import { generateSpaceSeed, InputSegment, PathResult, SearchQuotientNode } from './search-quotient-node.js'; | ||||||
| import { SearchQuotientSpur } from './search-quotient-spur.js'; | ||||||
|
|
||||||
| const PATH_QUEUE_COMPARATOR: Comparator<SearchQuotientNode> = (a, b) => { | ||||||
| return a.currentCost - b.currentCost; | ||||||
|
|
@@ -20,9 +21,10 @@ const PATH_QUEUE_COMPARATOR: Comparator<SearchQuotientNode> = (a, b) => { | |||||
| // The set of search spaces corresponding to the same 'context' for search. | ||||||
| // Whenever a wordbreak boundary is crossed, a new instance should be made. | ||||||
| export class SearchQuotientCluster implements SearchQuotientNode { | ||||||
| // While most functions can be done directly from SearchSpace, merging and splitting will need access | ||||||
| // to SearchPath-specific members. It's also cleaner to not allow nested SearchClusters while we | ||||||
| // haven't worked out support for such a scenario. | ||||||
| // While most functions can be done directly from SearchSpace, merging and | ||||||
| // splitting will need access to SearchQuotientSpur-specific members. It's | ||||||
| // also cleaner to not allow nested SearchQuotientClusters while we haven't | ||||||
| // worked out support for such a scenario. | ||||||
| private selectionQueue: PriorityQueue<SearchQuotientNode> = new PriorityQueue(PATH_QUEUE_COMPARATOR); | ||||||
| readonly spaceId: number; | ||||||
|
|
||||||
|
|
@@ -61,7 +63,7 @@ export class SearchQuotientCluster implements SearchQuotientNode { | |||||
| */ | ||||||
| constructor(inboundPaths: SearchQuotientNode[]) { | ||||||
| if(inboundPaths.length == 0) { | ||||||
| throw new Error("SearchCluster requires an array with at least one SearchPath"); | ||||||
| throw new Error("SearchQuotientCluster requires an array with at least one SearchQuotientNode"); | ||||||
| } | ||||||
|
|
||||||
| let lowestPossibleSingleCost = Number.POSITIVE_INFINITY; | ||||||
|
|
@@ -72,12 +74,12 @@ export class SearchQuotientCluster implements SearchQuotientNode { | |||||
|
|
||||||
| for(let path of inboundPaths) { | ||||||
| if(path.inputCount != inputCount || path.codepointLength != codepointLength) { | ||||||
| throw new Error(`SearchPath does not share same properties as others in the cluster: inputCount ${path.inputCount} vs ${inputCount}, codepointLength ${path.codepointLength} vs ${codepointLength}`); | ||||||
| throw new Error(`SearchQuotientNode does not share same properties as others in the cluster: inputCount ${path.inputCount} vs ${inputCount}, codepointLength ${path.codepointLength} vs ${codepointLength}`); | ||||||
| } | ||||||
|
|
||||||
| // If there's a source-range key mismatch - via mismatch in count or in actual ID, we have an error. | ||||||
| if(path.sourceRangeKey != sourceRangeKey) { | ||||||
| throw new Error(`SearchPath does not share the same source identifiers as others in the cluster`); | ||||||
| throw new Error(`SearchQuotientNode does not share the same source identifiers as others in the cluster`); | ||||||
| } | ||||||
|
|
||||||
| lowestPossibleSingleCost = Math.min(lowestPossibleSingleCost, path.lowestPossibleSingleCost); | ||||||
|
|
@@ -176,7 +178,52 @@ export class SearchQuotientCluster implements SearchQuotientNode { | |||||
| } | ||||||
|
|
||||||
| merge(space: SearchQuotientNode): SearchQuotientNode { | ||||||
| throw new Error('Method not implemented.'); | ||||||
| // If we're at a root (which is without inputs), bypass it. | ||||||
| if(space.parents.length == 0) { | ||||||
| return this; | ||||||
| } | ||||||
|
|
||||||
| // What if we're trying to merge something previously split? | ||||||
| // That can only happen at the head of the incoming space, so we check for it early here. | ||||||
| if(space.inputCount == 1 && space instanceof SearchQuotientSpur) { | ||||||
| // In such a case... the 'leading edge' of the incoming space needs to be checked | ||||||
| // against the trailing edge of `this` instance's entries. | ||||||
| const thisTailInputSource = this.inputSegments[this.inputSegments.length - 1]; | ||||||
| const thisTailSpaceIds = this.parents.map((path) => (path as SearchQuotientSpur).inputSource.subsetId); | ||||||
| const spaceHeadInputSource = space.inputSegments[0]; | ||||||
|
|
||||||
| const isOnSplitInput = | ||||||
| thisTailSpaceIds.find((entry) => entry == space.inputSource.subsetId) | ||||||
| && thisTailInputSource.end == spaceHeadInputSource.start; | ||||||
|
|
||||||
| // In this case, we only rebuild the single path; an outer stack frame will reconstitute | ||||||
| // the split cluster from the individual paths built here. | ||||||
| if(isOnSplitInput) { | ||||||
| const firstHalf = this.parents.find((tailPath) => (tailPath as SearchQuotientSpur).inputSource?.subsetId == space.inputSource?.subsetId); | ||||||
| return firstHalf.merge(space); | ||||||
| } | ||||||
| } | ||||||
|
|
||||||
| // Simple, straightforward. SearchQuotientSpurs can easily built with a | ||||||
| // SearchQuotientCluster as parent. In this case, there's also no chance of | ||||||
| // a prior split; if we'd split, it'd be a SearchQuotientCluster on both | ||||||
| // ends. | ||||||
| if(space instanceof SearchQuotientSpur) { | ||||||
| const parentMerge = this.merge(space.parents[0]) as SearchQuotientSpur; | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| return space.construct(parentMerge, space.inputs, space.inputSource); | ||||||
| } | ||||||
|
|
||||||
| // If we're here, we have a SearchQuotientCluster being merged in... and to | ||||||
| // something that's already a SearchQuotientCluster. | ||||||
| // | ||||||
| // Merge the parent components first as a baseline. This specific state's | ||||||
| // aspects have to come after their affects are merged in, anyway. | ||||||
| // (Note: is the main point of recursion.) | ||||||
| const parents = (space as SearchQuotientCluster).parents; // the constituent paths | ||||||
|
|
||||||
| // Assumes either none of the space's heads were split or that ALL were. | ||||||
| const parentMerges = parents.map((p) => this.merge(p)); // we get paths out | ||||||
| return new SearchQuotientCluster(parentMerges); | ||||||
| } | ||||||
|
|
||||||
| split(charIndex: number): [SearchQuotientNode, SearchQuotientNode] { | ||||||
|
|
||||||
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be better to use
Array.somehere. Theoreticallyspace.inputSource.subsetIdcan be 0 in which caseArray.findwould also return 0 i.e. falsy, and soisOnSplitInputwould befalseeven though it should betrue. I'm pretty sure this can't happen in our implementation (even though I don't know why), but usingArray.somewould be cleaner IMO (orthisTailSpaceIds.find(...) !== undefined)