Dr Mark C. Sinclair

Results 44 comments of Dr Mark C. Sinclair

It would be good to have an SIL boilerplate header on all source files (@mcdurdin says not strictly necessary): - `classical-calculation.ts` - `context-tracker.ts` - `distance-modeler.ts` - `execution-timer.ts` - `index.ts` -...

Starting by looking at `classical-calculation.ts`. Could we add a references to [Hamming](https://en.wikipedia.org/wiki/Hamming_distance), [Levenshtein](https://en.wikipedia.org/wiki/Levenshtein_distance) and [Damerau-Levenshtein](https://en.wikipedia.org/wiki/Damerau%E2%80%93Levenshtein_distance) in the [references](https://github.com/keymanapp/keyman/blob/69d162696a2175b6805e68bae31bf9dddfc267ce/web/src/engine/predictive-text/worker-thread/src/main/correction/classical-calculation.ts#L212C2-L215C39) for `ClassicalDistanceCalculation()` in `classical-calculation.ts`? The Hamming distance isn't strictly needed, but some...

If possible, could we add Javadoc comments to some additional functions/classes, such as: - [`getTrueIndex()`](https://github.com/keymanapp/keyman/blob/69d162696a2175b6805e68bae31bf9dddfc267ce/web/src/engine/predictive-text/worker-thread/src/main/correction/classical-calculation.ts#L309-L321) - [`getCostAt()`](https://github.com/keymanapp/keyman/blob/69d162696a2175b6805e68bae31bf9dddfc267ce/web/src/engine/predictive-text/worker-thread/src/main/correction/classical-calculation.ts#L323-L337) - [`initialCostAt()`](https://github.com/keymanapp/keyman/blob/69d162696a2175b6805e68bae31bf9dddfc267ce/web/src/engine/predictive-text/worker-thread/src/main/correction/classical-calculation.ts#L505-L524) - [`getSubset()`](https://github.com/keymanapp/keyman/blob/69d162696a2175b6805e68bae31bf9dddfc267ce/web/src/engine/predictive-text/worker-thread/src/main/correction/classical-calculation.ts#L526C3-L559) - [`addInputChar()`](https://github.com/keymanapp/keyman/blob/69d162696a2175b6805e68bae31bf9dddfc267ce/web/src/engine/predictive-text/worker-thread/src/main/correction/classical-calculation.ts#L561-L566) - [`_addInputChar()`](https://github.com/keymanapp/keyman/blob/69d162696a2175b6805e68bae31bf9dddfc267ce/web/src/engine/predictive-text/worker-thread/src/main/correction/classical-calculation.ts#L568-L585) - [`addMatchChar()`](https://github.com/keymanapp/keyman/blob/69d162696a2175b6805e68bae31bf9dddfc267ce/web/src/engine/predictive-text/worker-thread/src/main/correction/classical-calculation.ts#L587-L591) - [`_addMatchChar()`](https://github.com/keymanapp/keyman/blob/69d162696a2175b6805e68bae31bf9dddfc267ce/web/src/engine/predictive-text/worker-thread/src/main/correction/classical-calculation.ts#L593-L608) -...

As [`hasFinalCostWithin()`](https://github.com/keymanapp/keyman/blob/69d162696a2175b6805e68bae31bf9dddfc267ce/web/src/engine/predictive-text/worker-thread/src/main/correction/classical-calculation.ts#L373-L392) is only used in tests, should it be in a source file? Similarly, should [`visualizeCalculation()`](https://github.com/keymanapp/keyman/blob/69d162696a2175b6805e68bae31bf9dddfc267ce/web/src/engine/predictive-text/worker-thread/src/main/correction/classical-calculation.ts#L56-L158) and the commented-out [`visualize()`](https://github.com/keymanapp/keyman/blob/69d162696a2175b6805e68bae31bf9dddfc267ce/web/src/engine/predictive-text/worker-thread/src/main/correction/classical-calculation.ts#L789-L803) be in a source file? Perh aps they could...

Can you give some extra explanation for [`validate the match`](https://github.com/keymanapp/keyman/blob/69d162696a2175b6805e68bae31bf9dddfc267ce/web/src/engine/predictive-text/worker-thread/src/main/correction/classical-calculation.ts#L973-L980)? Is this just the check on line 974, or the `input == match` on 978?

It would be good to see some unit tests on: - [`findBaseEdges()`](https://github.com/keymanapp/keyman/blob/69d162696a2175b6805e68bae31bf9dddfc267ce/web/src/engine/predictive-text/worker-thread/src/main/correction/classical-calculation.ts#L952-L981) - [`findTransposeEdges()`](https://github.com/keymanapp/keyman/blob/69d162696a2175b6805e68bae31bf9dddfc267ce/web/src/engine/predictive-text/worker-thread/src/main/correction/classical-calculation.ts#L876-L938) - [`getSubset()`](https://github.com/keymanapp/keyman/blob/69d162696a2175b6805e68bae31bf9dddfc267ce/web/src/engine/predictive-text/worker-thread/src/main/correction/classical-calculation.ts#L526-L559) - [`_addInputChar()`](https://github.com/keymanapp/keyman/blob/69d162696a2175b6805e68bae31bf9dddfc267ce/web/src/engine/predictive-text/worker-thread/src/main/correction/classical-calculation.ts#L568-L585) - [`_addMatchChar()`](https://github.com/keymanapp/keyman/blob/69d162696a2175b6805e68bae31bf9dddfc267ce/web/src/engine/predictive-text/worker-thread/src/main/correction/classical-calculation.ts#L593-L608) - [`forNewIndices()`](https://github.com/keymanapp/keyman/blob/69d162696a2175b6805e68bae31bf9dddfc267ce/web/src/engine/predictive-text/worker-thread/src/main/correction/classical-calculation.ts#L816-L843) - [`increaseMaxDistance()`](https://github.com/keymanapp/keyman/blob/69d162696a2175b6805e68bae31bf9dddfc267ce/web/src/engine/predictive-text/worker-thread/src/main/correction/classical-calculation.ts#L610-L713) - [`propagateUpdateFrom()`](https://github.com/keymanapp/keyman/blob/69d162696a2175b6805e68bae31bf9dddfc267ce/web/src/engine/predictive-text/worker-thread/src/main/correction/classical-calculation.ts#L715-L773)

The code relating to `'merge'` ([here](https://github.com/keymanapp/keyman/blob/69d162696a2175b6805e68bae31bf9dddfc267ce/web/src/engine/predictive-text/worker-thread/src/main/correction/classical-calculation.ts#L126) and [here](https://github.com/keymanapp/keyman/blob/69d162696a2175b6805e68bae31bf9dddfc267ce/web/src/engine/predictive-text/worker-thread/src/main/correction/classical-calculation.ts#L438)) seems unused. Why is `'split'` not one of the [`EditOperations`](https://github.com/keymanapp/keyman/blob/69d162696a2175b6805e68bae31bf9dddfc267ce/web/src/engine/predictive-text/worker-thread/src/main/correction/classical-calculation.ts#L6)? I see that there is an `ExtendedEditOperation` in `segmentable-calculation.ts`, but this is...

The local variable [`totalId`](https://github.com/keymanapp/keyman/blob/69d162696a2175b6805e68bae31bf9dddfc267ce/web/src/engine/predictive-text/worker-thread/src/main/correction/classical-calculation.ts#L421) seems potentially confusing because of the usual meaning of `Id`. Might something like `totalInsertDelete` be clearer?

Could much of the recursive backtracing of paths (`PathBuilder`/`EdgeFinder`) not be replaced by storing the operations as the edge-distance array is built? You could then build them into paths more...

[`editpath()`](https://github.com/keymanapp/keyman/blob/69d162696a2175b6805e68bae31bf9dddfc267ce/web/src/engine/predictive-text/worker-thread/src/main/correction/classical-calculation.ts#L405-L487) could be replaced by a [Dijstra algorithm](https://en.wikipedia.org/wiki/Dijkstra%27s_algorithm) parameterised by a function to determine the operation costs (equivalent to link lengths in the original algorithm, one for matches, one for...