Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

@s1monw Do you have an opinion on those changes?

Multiply by alpha seems the right thing to do but I don't think that numTerms should be used. The frequency for each term is computed as the number of occurrences of this term across all documents so using the total term frequency for the entire corpus seems appropriate.
Also numTerms is unknown when the shard contains more than one segment so the value will be -1 most of the time (see #27149). I think we should always use the total term frequency and removes numTerms entirely from the formula.
Although I am not sure about the original intentions so I am also interested in what @s1monw thinks of this ;)

Multiply by alpha seems the right thing to do but I don't think that numTerms should be used. The frequency for each term is computed as the number of occurrences of this term across all documents so using the total term frequency for the entire corpus seems appropriate.

The idea of this smoothing comes from the MLE (Maximum Likelihood Estimation). We compute the probability of a term P(t1) as ttf(t1) / sum_ttf. In order to account for non existing terms which have some minor probability, we smooth the probability. In Laplace, aka Additive, you can add k where k is between 0 and 1. If you add 1 to a term in the numerator, you have to account that in the divider by adding one to all unique terms (to keep the probabilities the same). If you add k < 1 then you add k*numTerms to the divider. That's the probability theory behind it. If we'll add vocabSize == sum_ttf, we'll add a much larger value to the divider than we should, and therefore lower the probability of all other terms more than we want to.

Also numTerms is unknown when the shard contains more than one segment so the value will be -1 most of the time (see #27149).

I know that, and that's why I've also fixed numTerms to equal reader.maxDoc() as a (very low) approximation. But I think it's better than adding k * vocabSize since it will discount the probability of existing terms less.

Although I am not sure about the original intentions so I am also interested in what @s1monw thinks of this ;)

I don't necessarily know much about this anymore. I can totally look into it but that might be as good as your opinions.

Multiply by alpha seems the right thing to do but I don't think that numTerms should be used. The frequency for each term is computed as the number of occurrences of this term across all documents so using the total term frequency for the entire corpus seems appropriate.
Also numTerms is unknown when the shard contains more than one segment so the value will be -1 most of the time (see #27149). I think we should always use the total term frequency and removes numTerms entirely from the formula.

I tend to agree with this. The frequency for the bi-/tri-grams is calculated using a proportional value ie. docfreq or TTF if available. We use a proportional value for the vocabulary size, that seems correct to me.

@elasticmachine ok to test

Thanks @s1monw. Do you also have an opinion about my other question -- should LaplaceScorer also override scoreUnigram to implement k-additive smoothing (instead of falling back to add-one)?

Regarding whether to smooth by sum_ttf (aka vocabulary size) or numTerms, I still think it's wrong to divide by sum_ttf as it shaves off much more of the probability than you want in an add-k smoothing. E.g. take a look here: https://lagunita.stanford.edu/c4x/Engineering/CS-224N/asset/slp4.pdf#subsection.4.4.2, where V is previously defined as the "total extra observations", in our case the total number of unique terms.

Dividing by vocabSize == sum_ttf is just too high. On a not-so-large Wikipedia index (w/ English analysis, 5.6M docs), there are 5.4 billion term occurrences. I don't know what API exists to get the number of unique terms, but I believe it's much smaller, and I think that dividing by such a large number is not the right thing to do.

That that numTerms cannot always be computed is OK, as this patch also fixes it to default to reader.maxDoc() (which is needed for WordScorer.scoreUnigram() anyway), and it will mean it will shave off less probability.

Thoughts?

Regarding whether to smooth by sum_ttf (aka vocabulary size) or numTerms, I still think it's wrong to divide by sum_ttf as it shaves off much more of the probability than you want in an add-k smoothing. E.g. take a look here: https://lagunita.stanford.edu/c4x/Engineering/CS-224N/asset/slp4.pdf#subsection.4.4.2, where V is previously defined as the "total extra observations", in our case the total number of unique terms.

For the bigram/trigram case we use the relative frequency that for the bigram x_y is computed with sum_ttf(x_y) / sum_ttf(x) when ttf is available and tf(x_y) / tf(x) when it's not.
With add-k smoothing the formula is augmented with (sum_ttf(x_y) + k) / (sum_ttf(x) + kV).
For this case I agree that V can be maxDocs or numTerms, it's just an augmentation of the divider but the important part is that it's divided by the total term frequency of the prefix (n-1 gram) which is always greater than the sum_ttf of the entire ngram.
Sorry it was unclear but my comment was about the unigram case where we need to divide by total_term_freq of the entire corpus in order to get the probability of the term: sum_ttf(x) / ttf. It's not about the smoothing which can be any external observations as long as it's the same for all terms. So the formula for unigram would have to keep the ttf divider:
(sum_ttf(x) + k) / (ttf + kV) where V could be numTerms or maxDocs. Since numTerms is only available when a single segment is used I think it would be preferable to always use maxDocs but that's just for clarity. I am ok changing this formula if we fix the bigram/trigram case with your diff but the divider would be: vocabularySize + (k * maxDocs), keeping the total term frequency is important to make sure that the computed value is always <= 1.

The rest tests are failing due to the new formula:

14:40:44 FAILURE 0.09s | DocsClientYamlTestSuiteIT.test {yaml=reference/search/suggesters/phrase-suggest/line_86} <<< FAILURES!
14:40:44    > Throwable #1: java.lang.AssertionError: Failure at [reference/search/suggesters/phrase-suggest:103]: $body didn't match expected value:

Can you adapt these tests to the new formula ?

I am pretty sure that we are roughly on the same page, and since I'm not sure if you had typos in your response, I would like to re-iterate what you wrote:

for the bigram x_y is computed with sum_ttf(x_y) / sum_ttf(x) when ttf is available and tf(x_y) / tf(x) when it's not.

The formula that we use is (k + freq(x_y)) / (freq(x) + kV). freq() is computed either as ttf = total_term_freq or df = document freq. Just to be clear, sum_ttf denotes the sum of all term frequencies in a field, and ttf denotes the total term frequency of a single term.

but the important part is that it's divided by the total term frequency of the prefix (n-1 gram) which is always greater than the sum_ttf of the entire ngram.

Agreed. Except that it's divided by the frequency of the (n-1 gram), which is either ttf or df.

With that, I think we are on the same page, and I'll go fix LaplaceScorer to also override scoreUnigram and implement with add-k smoothing.

The rest tests are failing due to the new formula:

Hmm, it didn't fail when I ran gradle test. Should I run something else (maybe gradle itest :) )?

The formula that we use is (k + freq(x_y)) / (freq(x) + kV). freq() is computed either as ttf = total_term_freq or df = document freq. Just to be clear, sum_ttf denotes the sum of all term frequencies in a field, and ttf denotes the total term frequency of a single term.

We are on the same page, ok with the naming.

Agreed. Except that it's divided by the frequency of the (n-1 gram), which is either ttf or df.

Same here, naming issue. Thanks for using the right one ;).

Hmm, it didn't fail when I ran gradle test

To run all the tests you should use gradle check. gradle test only runs unit tests.

With that, I think we are on the same page, and I'll go fix LaplaceScorer to also override scoreUnigram and implement with add-k smoothing.

Ok. I am a bit concerned because it's the default scoring that we're changing but I am good if we consider this as a breaking change (same for the bigram/trigram change actually). The phrase_suggester is not easy to tune right so some users may rely on the current formula even though it was a bit buggy.

@jimczi I've fixed the code as we discussed. I also hope that I've addressed all test failures.

Thanks @jimczi!

Looks good guys thanks for fixing this @shaie