Add String.{edit_distance,spellcheck}, UTF-8 aware spellchecking functions

[dbuenzli]

Note to reviewers, there's quite a bit of stdlib related PR's these days I don't mind if this doesn't get reviewed for 5.4, I did it now because I had to go over this again and the material is in my mind.

This PR adds two new functions to the String module for devising “did you mean?” spell-checking functions on UTF-8 text:

val String.edit_distance : ?limit:int -> string -> string -> int 
val String.spellcheck : ?max_dist:(string -> int) -> string list -> string -> string list

Almost any program which deals with user provided input that needs to be matched against known strings can have its usability improved on errors by a simple mindless call to String.spellcheck, so it feels worth having them at hand in the Stdlib with good defaults.

The OCaml compiler itself has a form of these functions in Misc.edit_distance and Misc.spellcheck. This PR makes sure that at least the Misc.edit_function function can be replaced by the one proposed here. This in turn can fix the (unreported AFAIK) bug that these functions do not work meaningfully with the new modest support for Unicode letters in
identifiers.

String.edit_distance

Like Misc.edit_distance, we use the optimal string alignement distance except we do it on arrays of Uchar.t. In contrast to the Levenshtein distance, OSA allows two letter transpositions to be counted as 1 edit rather than 2. In contrast to Damerau-Levenshtein, OSA does not allow the same subsequence to be modified twice. We do not compute Damerau-Levenshtein distance as this requires a counter per letter occuring in the string which is a bit more costly and unconvenient given that, we work on Uchar.t letters (vs. say bytes), the data structures we have in the Stdlib, and the module dependency constraints.

Like in Misc.edit_distance we add a limit parameter to limit the computation to a distance threshold. In this case only the values on a stripe of length 2*limit+1 around the diagonal of the dynamic programming matrix need to be computed. This significantly reduces the work done when you are interested in selecting strings that are not further away than a given limit which is usually the case in spellchecking tasks.

Unlike Misc.edit_distance, the proposed function has an unconditional allocation cost due to the internal conversion to Uchar.t arrays on which the distance is effectively computed. To compensate this we reduce the dynamic programming matrix as is often suggested but not done in Misc.edit_distance (you only need a ring buffer of 3 rows) .

Since the raw implementation result may be a bit difficult to understand, its implementation is introduced by a succession of three commits.

1. The first commit adds a classical OSA distance computation between two UTF-8 strings. It should be easy to match this implementation on pseudo code or the recurrence definition of the dynamic programming matrix that can be found e.g. on wikipedia. By looking at the indexation operations in the loop it is also easy to see that you don't need the full dynamic programming matrix but only a ring buffer of three rows.

2. The second commit makes the optimisation of only keeping a ring buffer of three rows of the smallest string. This makes the space requirements of the dynamic programming matrix 3*length(s) with s the smallest string rather than length(s0)*length(s1).

3. The third commit adds a limit parameter to stop the computation once a maximal distance is reached. This involves computing the dynamic programming array only on a stripe of length 2*limit+1 around the diagonal and compute the minimal in a row for early exit (not present in Misc.edit_distance). This makes the algorithm O(limit*length(s)) time.

String.spellcheck

This is a ready made spell-checking function that can be used with small dictionaries represented by a list of string for correcting user input. Given a string s to correct it simply returns the list of words in the dictionary whose edit distance is the smallest and at most at a user provided max_dist s value. Note that this favours corrections to shorter words (this can be disputed but the aim here is to have an easy function one can quickly reach for. More elaborate schemes, e.g. based on word probability, can be devised by using the String.edit_distance primitive).

The max_dist function defaults to Fun.const 2 (this is no longer the case see discussion below). This default can be discussed, especially on small strings. The problem is that it likely depends on application usage. For example Misc.spellcheck adapts the distance according to identifiers length. I would have liked to find some pragmatic user study for a good default, but except for the internet rumor that users make generally no more than 2 of edit errors per word, nothing really came out. My aim with the current definition is that programmers can simply use the function without thinking and have a good enougth and easy to predict default behaviour. I mention in the docs that the default is subject to change, in case we do want to change it later (e.g. make it more like Misc.spellcheck).

Comments

• Note that in contrast to Misc.edit_distance which is able to bail out immediately on large string differences, String.edit_distance has to create the Uchar.t arrays for the strings before being able to bail out on that check. This has an impact on the gc behaviour of Misc.spellcheck's performance if env is large.

• It would have been nice to define edit_distance in the API of Array but we use it on subarrays here and we rather do not do an additional copy. This would have worked on Dynarray but I didn't want to introduce a dependency of String on Dynarray (though given that Dynarray are good Buffer.t for Uchar.t that's not entirely absurd). I'm willing to do this change if people think there's interest (i.e. have Dynarray.edit_distance ?equal:('a -> 'a -> bool) -> ?limit:int -> 'a t -> 'a t -> int) but I rather do the factorization in a subsequent PR.

• This PR adds a dependencies to String on the Int, List and Array modules (and Uchar but that was already a transitive dependency). Any of these dependencies can be removed (some at the cost of code repetition) Please tell me if there's the desire to trim any of these.

[wikku]

I'm not sure if it works with row-by-row iteration and distance functions with transpositions, but edit distance can be computed in O(nd), where d is the result, by starting with a stripe of length 1 and widening it as the result at the diagonal increases.

But maybe that's not necessary, as O(nd) can be achieved in a black box manner by starting with limit 1 and doubling it until the result no longer increases.

[gasche]

Thanks for the very complete, very careful PR description!

I haven't thought about it deeply yet, but here is a first round of feedback.

1. I wish that more tools had this "did you mean?" behavior on typos, so I support making it easy for other tool authors (using OCaml) to adopt. This being said, this could plausibly be done through a small opam package.

2. It is natural to ask whether the new stdlib support could be used in the compiler, replacing the code in Misc which could then be removed. (I suppose the motivation is also partly to have the compiler better support its new unicode identifiers; this interacts with (1) as it requires a compiler-libs or stdlib solution rather than a third-party opam package.) You probably have not done this, use your new stdlib code in the compiler, because it would require a bootstrap, which is more work than it should. (It would be fine to wait for the next OCaml release to do this change, for example.) But still, would we want to use your versions in the compiler? I would be nervous about performance, because we may run spellcheck on fairly large environments (set of defined names).

3. You mention that you haven't implemented an early bailout if the two strings have very-different sizes, which Misc has. Looking at the code, I see that you decode the string into a uchar array and compute the size at the same time. But you could also start by computing the size, to get the early bailout. My intuition (but I don't remember testing this in practice) is that it makes a performance difference for compiler-like use-case, where the environments can be large but most candidates bail out quickly. (Note: it's even possible to iterate on both strings in lockstep, to stop at size s + cutoff where s is the smallest of the two strings. I guess it is overkill, as iterating on strings to compute their uchar count is fast in practice.)

4. I wondered if it would be possible to take a fold:((string -> int -> int) -> int -> int) parameter instead of a string list parameter, to not force users to accumulate the environment into a list before calling the function. I thought that the Misc version did this already (given that the Env module in the compiler only provides folds), but I went to have a look again and in fact we use just a list. So maybe a list is fine.

[gasche]

Also: I would not have limit and max_dist as optional parameters, rather as mandatory parameters, with a recommendation to use ~max_dist:(fun _ -> 2) in the documentation to reduce decision fatigue. I think that this is a rather important choice as it noticeably impacts performance, so I would rather ensure that the user thinks about it.

[dbuenzli]

Thanks for the comments @gasche.

1. I wish that more tools had this "did you mean?" behavior on typos, so I support making it easy for other tool authors (using OCaml) to adopt. This being said, this could plausibly be done through a small opam package.

I would say that for those tools that are programmed in OCaml your wish and your suggestion are in contradiction :–)

Also: I would not have limit and max_dist as optional parameters, rather as mandatory parameters, with a recommendation to use ~max_dist:(fun _ -> 2) in the documentation to reduce decision fatigue. I think that this is a rather important choice as it noticeably impacts performance, so I would rather ensure that the user thinks about it.

I don't think limit should be a required parameter otherwise it's weird if you really want to compute the edit distance of two strings. Regarding max_dist it has a good performance default and I really rather not bother programmers with that (or that they end up making an unfortunate ergonomic decision). So again I prefer to have a good default, once they see result they are not happy with, they can start micro-managing by either tweaking max_dist or by devising their own spellcheck with the String.edit_distance primitive.

2. It is natural to ask whether the new stdlib support could be used in the compiler, replacing the code in Misc which could then be removed.

In fact I thought that the code in Misc could simply have a fast past for the early bail out on ASCII identifiers:

let edit_distance s0 s1 cutoff = 
  let is_ascii s = String.for_all Char.Ascii.is_valid s in
  if not (is_ascii s0 && is_ascii s1) 
  then String.edit_distance ~limit:cutoff s0 s1 else
  let len0 = String.length s0 and len1 = String.length s1 in
  let limit = Int.min (Int.max len0 len1) cutoff in
  if Int.abs (len1 - len0) > limit then limit else 
  String.edit_distance ~limit:cutoff s0 s1

For now I opted not to do this in String.edit_distance in order not to make cultural assumptions :–) But this decision can certainly be revised, we could also, as you suggest in point 3. simply try to compute the UTF-8 length without decoding which may not be that different of the performance of is_ascii.

3. You mention that you haven't implemented an early bailout if the two strings have very-different sizes, which Misc has.

Just to make things clear, I do this but the check is done after the strings have been decoded to Uchar.t arrays and thus allocation of the arrays has occurred. See previous point.

4. I wondered if it would be possible to take a fold:((string -> int -> int) -> int -> int) parameter instead of a string list parameter,

I rather have a simple interface. Sophisticated users can reuse the nice primitive String.edit_distance. The code of String.spellcheck is rather straightforward (but I'd like to have it because the effort to think about it and to write it down may make programmers abandon the idea of implementing a “did you mean?”, I want “did you mean?” in the simplest OCaml scripts).

[dbuenzli]

So just to give an idea I made a toy benchmark which computes the distance hello with limits 2,3,4 against the 235976 words of my /usr/share/dict/words (so these timing represent ~700'000 checks against purely ASCII words).

Here are the numbers, the first run corresponds to the String.edit_distance of this PR (as found in B0_std, it's exactly the same code), the second uses a cut and paste of Misc.edit_distance and the third one is the function with the ASCII fast path mentioned in my previous message:

> hyperfine "$(brzo -b --path)"
Benchmark 1: /Users/dbuenzli/tmp/eddist/_b0/brzo/ocaml-exec-native/a.out
  Time (mean ± σ):     100.3 ms ±   0.5 ms    [User: 96.2 ms, System: 3.4 ms]
  Range (min … max):    99.6 ms … 101.9 ms    28 runs
 
> hyperfine "$(brzo -b --path) --use-misc"
Benchmark 1: /Users/dbuenzli/tmp/eddist/_b0/brzo/ocaml-exec-native/a.out --use-misc
  Time (mean ± σ):      95.0 ms ±   0.6 ms    [User: 90.5 ms, System: 3.7 ms]
  Range (min … max):    93.9 ms …  96.5 ms    30 runs
 
> hyperfine "$(brzo -b --path) --use-ascii-path"
Benchmark 1: /Users/dbuenzli/tmp/eddist/_b0/brzo/ocaml-exec-native/a.out --use-ascii-path
  Time (mean ± σ):      87.6 ms ±   0.5 ms    [User: 83.5 ms, System: 3.4 ms]
  Range (min … max):    86.3 ms …  88.8 ms    33 runs

[dbuenzli]

And just to round that up. I tried to compute the Uchar.t length first for the early bail out (via this patch) but at least on the ASCII benchmark above that came out slower:

> hyperfine "$(brzo -b --path)"
Benchmark 1: /Users/dbuenzli/tmp/eddist/_b0/brzo/ocaml-exec-native/a.out
  Time (mean ± σ):     106.9 ms ±   0.6 ms    [User: 102.5 ms, System: 3.6 ms]
  Range (min … max):   105.3 ms … 107.8 ms    27 runs

[gasche]

Thanks! Your benchmark computes edit distance three times. What is the performance of the 0-times version, that just does the rest of the logic? (This helps estimate relative differences in efficiency between the variants you measure.)

The ascii version is quite nice; today all identifiers are ascii so this is just fine, but I'm thinking than in the future it is more likely that the user-provided identifier may have unicode, while the environment will be mostly ascii-only. In the use-case of computing the edit distance of one string against many, one "needle" in a haystack of other strings, (one could use a clever automata, anyways) we could in fact compute the length of that needle once, which would provide a significant speedup in the utf8-length-bailout version. But this makes the interface slightly uglier (providing an optional parameter for the uchar length of each argument?), so I'm not too convinced myself that this is a good idea.

[dbuenzli]

Here's the cost of just loading and iterating over the words of the dictionary:

> hyperfine "$(brzo -b --path) --just-iter"
Benchmark 1: /Users/dbuenzli/tmp/eddist/_b0/brzo/ocaml-exec-native/a.out --just-iter
  Time (mean ± σ):      20.6 ms ±   0.3 ms    [User: 17.8 ms, System: 2.2 ms]
  Range (min … max):    20.1 ms …  21.7 ms    126 runs

[gasche]

I'm satisfied with your benchmarking work: I think the ascii bailout is a reasonable approach, we could have it in Misc as your suggest or include it in Stdlib if you agree (this would have my mild preference, but no pressure).

Looking at the Misc implementation again, I see that the max_dist function we use (rephrased in terms of your API) is the following:

  let max_dist name =
    match String.length name with
      | 1 | 2 -> 0
      | 3 | 4 -> 1
      | 5 | 6 -> 2
      | _ -> 3

You propose (fun _ -> 2) instead by default. This seems to have worse behavior in the case of one-uchar strings, where all other one-uchar strings would be returned as suggestions (whereas Misc disables spell-checking entirely for those characters). If the intention is to have a good default for catching human-made typos, I would maybe suggest (1 | 2) -> 0 | (3 | 4) -> 1 | _ -> 2.

[dbuenzli]

we could have it in Misc as your suggest or include it in Stdlib if you agree (this would have my mild preference, but no pressure).

Why not. But I'd like to also benchmark with an international list of words.

I would maybe suggest (1 | 2) -> 0 | (3 | 4) -> 1 | _ -> 2.

Yes. (I wanted to avoid to compute the Unicode length in the default but it's misguided, it's better to have this).

[dbuenzli]

So I changed the default of String.spellcheck as suggesed by @gasche. I will move and expose the uchar_utf_8_decode_length_of_byte function to Uchar in a subsequent PR, if this gets merged. It's a useful function to have in the stdlib.

Now regarding adding the optimization for ASCII by default I think I rather not. It's (rather obviously) slower as soon as there are characters beyond ASCII and the function advertises as working on Unicode, see numbers below.

Besides String.edit_distance is likely faster anyways than the current¹ Misc. An empirical observation seems to indicate Misc becomes slower when the checked word reaches 8 characters. I suspect you do pay a lot for the full matrix when the words grow larger. For example testing the word List.fald_left against the /usr/share/dict/words (I tried to convince ocp-index to dump me all the qualified names it knows but I failed, if someone knows a trick, let me know) we get:

Benchmark 1: /Users/dbuenzli/tmp/eddist/_b0/brzo/ocaml-exec-native/a.out --use-proposal --word List.fald_left
  Time (mean ± σ):     136.0 ms ±   0.7 ms    [User: 131.2 ms, System: 4.0 ms]
  Range (min … max):   134.3 ms … 138.3 ms    21 runs
 
Benchmark 2: /Users/dbuenzli/tmp/eddist/_b0/brzo/ocaml-exec-native/a.out --use-ascii --word List.fald_left
  Time (mean ± σ):     123.4 ms ±   0.6 ms    [User: 118.6 ms, System: 4.0 ms]
  Range (min … max):   122.6 ms … 125.3 ms    23 runs
 
Benchmark 3: /Users/dbuenzli/tmp/eddist/_b0/brzo/ocaml-exec-native/a.out --use-misc --word List.fald_left
  Time (mean ± σ):     228.1 ms ±   0.5 ms    [User: 221.5 ms, System: 5.5 ms]
  Range (min … max):   227.3 ms … 229.0 ms    12 runs
 
Benchmark 4: /Users/dbuenzli/tmp/eddist/_b0/brzo/ocaml-exec-native/a.out --just-iter --word List.fald_left
  Time (mean ± σ):      20.5 ms ±   0.2 ms    [User: 17.8 ms, System: 2.3 ms]
  Range (min … max):    20.1 ms …  21.4 ms    128 runs
 
Summary
  /Users/dbuenzli/tmp/eddist/_b0/brzo/ocaml-exec-native/a.out --just-iter --word List.fald_left ran
    6.01 ± 0.06 times faster than /Users/dbuenzli/tmp/eddist/_b0/brzo/ocaml-exec-native/a.out --use-ascii --word List.fald_left
    6.62 ± 0.07 times faster than /Users/dbuenzli/tmp/eddist/_b0/brzo/ocaml-exec-native/a.out --use-proposal --word List.fald_left
   11.11 ± 0.10 times faster than /Users/dbuenzli/tmp/eddist/_b0/brzo/ocaml-exec-native/a.out --use-misc --word List.fald_left

Since I found a way to dump aspell dictionaries as word lists. I did a bit more testing on Unicode data.

The following benchmark is done on the serbian dictionary which has the advantage of having both the cyrillic and latin script words of the language. This means we get a mix of words made of only ASCII characters, ASCII characters and multibyte characters and only multibyte character (686'453 words in total, the timings below are for ~2 million edit distance computations).

Here are the results against a pure ASCII word (funkcija) followed by the same word in the cyrillic script (функција) (note that we are faster than Misc, but Misc is meaningless anyways on these):

Benchmark 1: /Users/dbuenzli/tmp/eddist/_b0/brzo/ocaml-exec-native/a.out --use-proposal --dict-file /tmp/dict-sr --word funkcija
  Time (mean ± σ):     441.3 ms ±   2.0 ms    [User: 425.6 ms, System: 13.0 ms]
  Range (min … max):   437.1 ms … 444.9 ms    10 runs
 
Benchmark 2: /Users/dbuenzli/tmp/eddist/_b0/brzo/ocaml-exec-native/a.out --use-ascii --dict-file /tmp/dict-sr --word funkcija
  Time (mean ± σ):     479.1 ms ±   0.9 ms    [User: 463.8 ms, System: 12.7 ms]
  Range (min … max):   478.1 ms … 481.0 ms    10 runs
 
Benchmark 3: /Users/dbuenzli/tmp/eddist/_b0/brzo/ocaml-exec-native/a.out --use-misc --dict-file /tmp/dict-sr --word funkcija
  Time (mean ± σ):     476.3 ms ±   1.3 ms    [User: 460.9 ms, System: 12.3 ms]
  Range (min … max):   474.8 ms … 479.4 ms    10 runs
 
Benchmark 4: /Users/dbuenzli/tmp/eddist/_b0/brzo/ocaml-exec-native/a.out --just-iter --dict-file /tmp/dict-sr --word funkcija
  Time (mean ± σ):      58.8 ms ±   0.3 ms    [User: 52.1 ms, System: 5.3 ms]
  Range (min … max):    58.4 ms …  60.6 ms    47 runs
 
Summary
  /Users/dbuenzli/tmp/eddist/_b0/brzo/ocaml-exec-native/a.out --just-iter --dict-file /tmp/dict-sr --word funkcija ran
    7.50 ± 0.05 times faster than /Users/dbuenzli/tmp/eddist/_b0/brzo/ocaml-exec-native/a.out --use-proposal --dict-file /tmp/dict-sr --word funkcija
    8.10 ± 0.05 times faster than /Users/dbuenzli/tmp/eddist/_b0/brzo/ocaml-exec-native/a.out --use-misc --dict-file /tmp/dict-sr --word funkcija
    8.14 ± 0.05 times faster than /Users/dbuenzli/tmp/eddist/_b0/brzo/ocaml-exec-native/a.out --use-ascii --dict-file /tmp/dict-sr --word funkcija

Benchmark 1: /Users/dbuenzli/tmp/eddist/_b0/brzo/ocaml-exec-native/a.out --use-proposal --dict-file /tmp/dict-sr --word функцијаb
  Time (mean ± σ):     470.7 ms ±   1.7 ms    [User: 455.3 ms, System: 13.2 ms]
  Range (min … max):   468.5 ms … 473.6 ms    10 runs
 
Benchmark 2: /Users/dbuenzli/tmp/eddist/_b0/brzo/ocaml-exec-native/a.out --use-ascii --dict-file /tmp/dict-sr --word функција
  Time (mean ± σ):     480.7 ms ±   1.6 ms    [User: 464.3 ms, System: 13.7 ms]
  Range (min … max):   477.0 ms … 482.3 ms    10 runs
 
Benchmark 3: /Users/dbuenzli/tmp/eddist/_b0/brzo/ocaml-exec-native/a.out --use-misc --dict-file /tmp/dict-sr --word функција
  Time (mean ± σ):     725.5 ms ±   1.3 ms    [User: 704.3 ms, System: 18.6 ms]
  Range (min … max):   723.6 ms … 727.6 ms    10 runs
 
Benchmark 4: /Users/dbuenzli/tmp/eddist/_b0/brzo/ocaml-exec-native/a.out --just-iter --dict-file /tmp/dict-sr --word функција
  Time (mean ± σ):      59.3 ms ±   0.5 ms    [User: 52.2 ms, System: 5.7 ms]
  Range (min … max):    58.4 ms …  60.8 ms    47 runs
 
Summary
  /Users/dbuenzli/tmp/eddist/_b0/brzo/ocaml-exec-native/a.out --just-iter --dict-file /tmp/dict-sr --word функција ran
    7.94 ± 0.08 times faster than /Users/dbuenzli/tmp/eddist/_b0/brzo/ocaml-exec-native/a.out --use-proposal --dict-file /tmp/dict-sr --word функција
    8.11 ± 0.08 times faster than /Users/dbuenzli/tmp/eddist/_b0/brzo/ocaml-exec-native/a.out --use-ascii --dict-file /tmp/dict-sr --word функција
   12.24 ± 0.12 times faster than /Users/dbuenzli/tmp/eddist/_b0/brzo/ocaml-exec-native/a.out --use-misc --dict-file /tmp/dict-sr --word функција

Footnotes

1. It would be of course be easy to make it faster than what is proposed here, I assume we are interested in not having a regression with the status quo. ↩

[dbuenzli]

Just FTR I have overcome my indexing fears and have implemented in the benchmark a version which uses 3 buffers of 2 * limit + 2 length for the matrix.

FWIW it came out slower on the serbian dictionary benchmark; I suspect you pay for more branches and natural language words are rather small so the buffer sizes are not that different from those of the smaller word (however a function like spellcheck could reuse the buffers).

It's also strictly more index fiddly and less readable. It could make a difference on long identifiers though but for now I decided to leave things as they are. It seems the performance tradeoffs are quite data dependent.

[gasche]

I'm convinced by your performance measurements and feel okay with letting you decide whether to have the fast path in the stdlib version or not -- you are the expert. I didn't mean to throw you into an infinite iterative-deepening benchmarking loop.

[dbuenzli]

Oh don't worry that's self-imposed procrastination :–) I'd still like to run the bench on the dictionary of all qualified names of my opam switch, but that will have to wait a bit.

[gasche]

(I thought that using this in utils/misc.ml would require a bootstrap, but in fact I was mistaken, I don't think we need a bootstrap to use the code in stdlib/foo.ml.)

[dbuenzli]

So I have a small script to dump all the qualified identifiers of an opam switch with ocp-index. On a switch with ~100 packages which includes alternate standard libraries (except base, somehow ocp-index explodes when base is installed) that's about 427'504 ASCII identifiers (and thus the benchmark makes ~1.3Mio edit distance computation).

Here again the results are quite data dependent, with short identifiers Misc is much faster. I computed the histogram and the quartiles of the length of identifiers and the first quartile (25%) comes out at length of 24. But I couldn't quickly figure out if in the compiler the edit distance is in fact really computed on fully qualified names.

There are a few things that could be tried (e.g. rather than benchmark edit_distance benchmark spellcheck, have the windowed version and have spellcheck reuse window buffers and the Uchar.t conversion of the word to correct) but if I don't get the time to get back to this also feel that this could be done later and the PR merged as is. Also given the absolute timing numbers and the number of edit distance computations this represents I would personally not be nervous about replacing Misc.edit_distance by String.edit_distance.

So here are a few results on the "opam dictionary", with fald_left, List.fald_left and an identifier that has the quartile length 24 123456789012345678901234 (I added all the data and code mentioned in this PR in the gist if people want to have a closer look).

> b0 -- bench-dict-opam --word=fald_left          
Benchmark 1: /Users/dbuenzli/tmp/eddist/_b0/b/user/test-edit-distance/test-edit-distance --use-proposal --dict-file dict-opam --word=fald_left
  Time (mean ± σ):     335.9 ms ±   1.3 ms    [User: 320.2 ms, System: 13.9 ms]
  Range (min … max):   334.0 ms … 337.7 ms    10 runs
 
Benchmark 2: /Users/dbuenzli/tmp/eddist/_b0/b/user/test-edit-distance/test-edit-distance --use-window --dict-file dict-opam --word=fald_left
  Time (mean ± σ):     336.1 ms ±   1.5 ms    [User: 320.2 ms, System: 13.3 ms]
  Range (min … max):   334.3 ms … 338.8 ms    10 runs
 
Benchmark 3: /Users/dbuenzli/tmp/eddist/_b0/b/user/test-edit-distance/test-edit-distance --use-ascii --dict-file dict-opam --word=fald_left
  Time (mean ± σ):     139.4 ms ±   0.4 ms    [User: 129.6 ms, System: 8.3 ms]
  Range (min … max):   138.9 ms … 140.6 ms    21 runs
 
Benchmark 4: /Users/dbuenzli/tmp/eddist/_b0/b/user/test-edit-distance/test-edit-distance --use-misc --dict-file dict-opam --word=fald_left
  Time (mean ± σ):      70.5 ms ±   0.7 ms    [User: 62.5 ms, System: 6.6 ms]
  Range (min … max):    69.3 ms …  72.9 ms    41 runs
 
Benchmark 5: /Users/dbuenzli/tmp/eddist/_b0/b/user/test-edit-distance/test-edit-distance --just-iter --dict-file dict-opam --word=fald_left
  Time (mean ± σ):      48.6 ms ±   0.8 ms    [User: 40.9 ms, System: 6.2 ms]
  Range (min … max):    47.4 ms …  50.9 ms    59 runs
 
Summary
  /Users/dbuenzli/tmp/eddist/_b0/b/user/test-edit-distance/test-edit-distance --just-iter --dict-file dict-opam --word=fald_left ran
    1.45 ± 0.03 times faster than /Users/dbuenzli/tmp/eddist/_b0/b/user/test-edit-distance/test-edit-distance --use-misc --dict-file dict-opam --word=fald_left
    2.87 ± 0.04 times faster than /Users/dbuenzli/tmp/eddist/_b0/b/user/test-edit-distance/test-edit-distance --use-ascii --dict-file dict-opam --word=fald_left
    6.91 ± 0.11 times faster than /Users/dbuenzli/tmp/eddist/_b0/b/user/test-edit-distance/test-edit-distance --use-proposal --dict-file dict-opam --word=fald_left
    6.91 ± 0.11 times faster than /Users/dbuenzli/tmp/eddist/_b0/b/user/test-edit-distance/test-edit-distance --use-window --dict-file dict-opam --word=fald_left

> b0 -- bench-dict-opam --word=List.fald_left
Benchmark 1: /Users/dbuenzli/tmp/eddist/_b0/b/user/test-edit-distance/test-edit-distance --use-proposal --dict-file dict-opam --word=List.fald_left
  Time (mean ± σ):     372.3 ms ±   1.0 ms    [User: 358.2 ms, System: 11.9 ms]
  Range (min … max):   370.5 ms … 373.3 ms    10 runs
 
Benchmark 2: /Users/dbuenzli/tmp/eddist/_b0/b/user/test-edit-distance/test-edit-distance --use-window --dict-file dict-opam --word=List.fald_left
  Time (mean ± σ):     368.6 ms ±   1.6 ms    [User: 355.1 ms, System: 11.3 ms]
  Range (min … max):   366.6 ms … 371.6 ms    10 runs
 
Benchmark 3: /Users/dbuenzli/tmp/eddist/_b0/b/user/test-edit-distance/test-edit-distance --use-ascii --dict-file dict-opam --word=List.fald_left
  Time (mean ± σ):     172.8 ms ±   0.8 ms    [User: 162.9 ms, System: 8.3 ms]
  Range (min … max):   172.1 ms … 175.1 ms    16 runs
 
Benchmark 4: /Users/dbuenzli/tmp/eddist/_b0/b/user/test-edit-distance/test-edit-distance --use-misc --dict-file dict-opam --word=List.fald_left
  Time (mean ± σ):     149.3 ms ±   0.5 ms    [User: 140.3 ms, System: 7.7 ms]
  Range (min … max):   148.4 ms … 150.3 ms    19 runs
 
Benchmark 5: /Users/dbuenzli/tmp/eddist/_b0/b/user/test-edit-distance/test-edit-distance --just-iter --dict-file dict-opam --word=List.fald_left
  Time (mean ± σ):      48.3 ms ±   0.4 ms    [User: 40.9 ms, System: 6.1 ms]
  Range (min … max):    47.8 ms …  50.1 ms    59 runs
 
Summary
  /Users/dbuenzli/tmp/eddist/_b0/b/user/test-edit-distance/test-edit-distance --just-iter --dict-file dict-opam --word=List.fald_left ran
    3.09 ± 0.03 times faster than /Users/dbuenzli/tmp/eddist/_b0/b/user/test-edit-distance/test-edit-distance --use-misc --dict-file dict-opam --word=List.fald_left
    3.58 ± 0.03 times faster than /Users/dbuenzli/tmp/eddist/_b0/b/user/test-edit-distance/test-edit-distance --use-ascii --dict-file dict-opam --word=List.fald_left
    7.63 ± 0.07 times faster than /Users/dbuenzli/tmp/eddist/_b0/b/user/test-edit-distance/test-edit-distance --use-window --dict-file dict-opam --word=List.fald_left
    7.71 ± 0.07 times faster than /Users/dbuenzli/tmp/eddist/_b0/b/user/test-edit-distance/test-edit-distance --use-proposal --dict-file dict-opam --word=List.fald_left

> b0 -- bench-dict-opam --word=123456789012345678901234
Benchmark 1: /Users/dbuenzli/tmp/eddist/_b0/b/user/test-edit-distance/test-edit-distance --use-proposal --dict-file dict-opam --word=123456789012345678901234
  Time (mean ± σ):     432.3 ms ±  11.3 ms    [User: 417.1 ms, System: 13.0 ms]
  Range (min … max):   408.8 ms … 446.6 ms    10 runs
 
Benchmark 2: /Users/dbuenzli/tmp/eddist/_b0/b/user/test-edit-distance/test-edit-distance --use-window --dict-file dict-opam --word=123456789012345678901234
  Time (mean ± σ):     418.4 ms ±   8.4 ms    [User: 403.5 ms, System: 12.5 ms]
  Range (min … max):   402.4 ms … 429.4 ms    10 runs
 
Benchmark 3: /Users/dbuenzli/tmp/eddist/_b0/b/user/test-edit-distance/test-edit-distance --use-ascii --dict-file dict-opam --word=123456789012345678901234
  Time (mean ± σ):     248.6 ms ±   2.6 ms    [User: 237.8 ms, System: 9.2 ms]
  Range (min … max):   244.6 ms … 251.1 ms    12 runs
 
Benchmark 4: /Users/dbuenzli/tmp/eddist/_b0/b/user/test-edit-distance/test-edit-distance --use-misc --dict-file dict-opam --word=123456789012345678901234
  Time (mean ± σ):     367.3 ms ±   0.8 ms    [User: 353.0 ms, System: 12.4 ms]
  Range (min … max):   366.3 ms … 368.5 ms    10 runs
 
Benchmark 5: /Users/dbuenzli/tmp/eddist/_b0/b/user/test-edit-distance/test-edit-distance --just-iter --dict-file dict-opam --word=123456789012345678901234
  Time (mean ± σ):      48.2 ms ±   0.5 ms    [User: 40.9 ms, System: 6.0 ms]
  Range (min … max):    47.4 ms …  50.2 ms    59 runs
 
Summary
  /Users/dbuenzli/tmp/eddist/_b0/b/user/test-edit-distance/test-edit-distance --just-iter --dict-file dict-opam --word=123456789012345678901234 ran
    5.16 ± 0.07 times faster than /Users/dbuenzli/tmp/eddist/_b0/b/user/test-edit-distance/test-edit-distance --use-ascii --dict-file dict-opam --word=123456789012345678901234
    7.62 ± 0.08 times faster than /Users/dbuenzli/tmp/eddist/_b0/b/user/test-edit-distance/test-edit-distance --use-misc --dict-file dict-opam --word=123456789012345678901234
    8.68 ± 0.19 times faster than /Users/dbuenzli/tmp/eddist/_b0/b/user/test-edit-distance/test-edit-distance --use-window --dict-file dict-opam --word=123456789012345678901234
    8.97 ± 0.25 times faster than /Users/dbuenzli/tmp/eddist/_b0/b/user/test-edit-distance/test-edit-distance --use-proposal --dict-file dict-opam --word=123456789012345678901234

[gasche]

I agree than taking less than half a second on all names exported by all OCaml compilation units in opam is no cause for concern. In your List.fald_left test, your code is about 3x slower than Misc (if we substract the base iteration time), only 1.3x slower with the ascii fast path, and this is an acceptable code to pay in exchange for the warm fuzzy feeling of handling all of unicode correctly.

[gasche]

Approved. (This is a stdlib change so it will need a second maintainer approval.)

I have not in fact reviewed the core implementation in details, but I'm broadly confident and there is a nice testsuite.

Minor comment: currently you reuse the Misc interface relatively to the limit parameter, that you cap the return value with limit. I briefly wondered if it wouldn't be nicer to return None in that case (when we know that the distance is at least limit, but do not know for sure that it is limit). But this makes the API less pleasant to use for people who don't provide a limit, so I guess it's not such a good idea. Oh well...

[dbuenzli]

But this makes the API less pleasant to use for people who don't provide a limit, so I guess it's not such a good idea. Oh well...

Having None is also strictly more annoying if you are trying to compute minimums, see String.spellcheck.

In the end I couldn't get out of my mind that spellcheck was converting the spellchecked word to uchars for each word of the dictionary so in 70ae9eb I factored out this computation (a bench below, see the reuse timings, this is a use of spellcheck so it's more indicative of the time taken on a concrete dict than earlier benchmarks).

> b0 -- bench-spellcheck-dict-opam --word функција
Benchmark 1: /Users/dbuenzli/tmp/eddist/_b0/b/user/test-spellcheck/test-spellcheck --use-proposal --dict-file dict-opam --word функција
  Time (mean ± σ):     148.1 ms ±   0.9 ms    [User: 139.5 ms, System: 7.0 ms]
  Range (min … max):   147.1 ms … 150.0 ms    19 runs
 
Benchmark 2: /Users/dbuenzli/tmp/eddist/_b0/b/user/test-spellcheck/test-spellcheck --use-reuse --dict-file dict-opam --word функција
  Time (mean ± σ):     110.9 ms ±   1.5 ms    [User: 103.1 ms, System: 6.4 ms]
  Range (min … max):   107.2 ms … 114.6 ms    26 runs
 
Summary
  /Users/dbuenzli/tmp/eddist/_b0/b/user/test-spellcheck/test-spellcheck --use-reuse --dict-file dict-opam --word функција ran
    1.34 ± 0.02 times faster than /Users/dbuenzli/tmp/eddist/_b0/b/user/test-spellcheck/test-spellcheck --use-proposal --dict-file dict-opam --word функција
> b0 -- bench-spellcheck-dict-sr --word функција
Benchmark 1: /Users/dbuenzli/tmp/eddist/_b0/b/user/test-spellcheck/test-spellcheck --use-proposal --dict-file dict-sr --word функција
  Time (mean ± σ):     128.3 ms ±   0.6 ms    [User: 120.4 ms, System: 6.4 ms]
  Range (min … max):   127.4 ms … 129.8 ms    22 runs
 
Benchmark 2: /Users/dbuenzli/tmp/eddist/_b0/b/user/test-spellcheck/test-spellcheck --use-reuse --dict-file dict-sr --word функција
  Time (mean ± σ):     111.5 ms ±   0.9 ms    [User: 103.7 ms, System: 6.3 ms]
  Range (min … max):   110.8 ms … 115.4 ms    26 runs
 
Summary
  /Users/dbuenzli/tmp/eddist/_b0/b/user/test-spellcheck/test-spellcheck --use-reuse --dict-file dict-sr --word функција ran
    1.15 ± 0.01 times faster than /Users/dbuenzli/tmp/eddist/_b0/b/user/test-spellcheck/test-spellcheck --use-proposal --dict-file dict-sr --word функција

[nojb]

LGTM (did not read the algorithm itself in detail).

Happy to see non-trivial functions enter the standard library!

[gasche]

We have two maintainers approval, but I wonder if @Octachron wants to have a look before we merge. (As our resident second unicode-and-also-edit-distances nerd, or in case he has opinions on dependencies among stdlib modules.) @Octachron, could you in the next few days tell us whether you want to have a look (then we can of course wait longer) or are happy to let us merge right now?

[Octachron]

Yes, I am planning to have a look at the beginning of next week.

[Octachron]

The current implementation state looks good to me, thanks for enduring my nitpicking !