feat(web): implement tokenization convergence and associated unit test 🚂#15834
feat(web): implement tokenization convergence and associated unit test 🚂#15834jahorton wants to merge 1 commit intorefactor/web/split-analyze-transitionfrom
Conversation
Build-bot: skip build:web Test-bot: skip
User Test ResultsTest specification and instructions User tests are not required Test Artifacts
|
keymanapp-test-bot
bot
changed the title
mcdurdin
left a comment
mcdurdin
left a comment
There was a problem hiding this comment.
I am a little lost by some of the terminology, but otherwise LGTM
| finalizedTokens.push(bucket[0]); | ||
| } else { | ||
| const constituentSpurs = bucket.flatMap((token) => { | ||
| const quotientNode = token.searchModule; |
There was a problem hiding this comment.
I don't really understand all the terminology here: searchModule doesn't seem to have anything to do with quotientNode?
Even this function refers to a lot of concepts which I am struggling to put together into a coherent model: tokens (vs tokenizations), buckets, nodes, spurs, [search?]modules, clusters, quotients.
There was a problem hiding this comment.
I probably should do a quality pass for consistency on many of the properties, class names, and such once all is said and done. For one, some things were put in place before #15161 was developed and put in place. I haven't enforced the new terms in all of the pre-existing code.
(In regard to this specific case: quotient graphs are graphs based on "modules" of a more detailed graph, to visualize the higher-level patterns found within.)
Not aiming to dismiss your concerns here whatsoever; it's been a journey working out the terms as work proceeds, and I hope there will be sufficient time to "polish things up" in this regard before release.
| * An error will be thrown if the instances do not sufficiently converge to the | ||
| * same tokenization pattern. |
There was a problem hiding this comment.
See SearchQuotientCluster's constructor. This comment is regarding the error that will be generated within that constructor for such cases.
There was a problem hiding this comment.
| * An error will be thrown if the instances do not sufficiently converge to the | ||
| * same tokenization pattern. |
There was a problem hiding this comment.
I am not seeing any error thrown in this function
3 participants
Now that the
SearchQuotientClustertype exists, and as we now have a way to directly test conditions in which multiple search paths converge in the quotient-graph perspective, it's a good time to actually implement the code needed to buildSearchQuotientClusters during context transitions... and to add at least one unit test while we're at it.Build-bot: skip build:web
Test-bot: skip