Skip to content
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

Ranking: include filename matches in bm25 #757

Merged
merged 5 commits into from
Apr 17, 2024
Merged

Ranking: include filename matches in bm25 #757

merged 5 commits into from
Apr 17, 2024

Conversation

jtibshirani
Copy link
Member

BM25 considers a file to be a better match when there are many occurrences of
terms in the file. It's important to count all term occurrences, including
those in other fields like the filename.

For historical reasons, Zoekt trims all filename matches from a result if there
are any content matches. This meant that in BM25 scoring, we didn't account for
filename matches.

This PR refactors the match code so that we only trim filename matches when
assembling the final FileMatch. We retain filename matches when creating
candidateMatch, which lets BM25 scoring use them. Even without the better
BM25 scoring, I think this refactor makes the code easier to follow.

@cla-bot cla-bot bot added the cla-signed label Apr 16, 2024
@@ -127,14 +130,24 @@ func (d *indexData) scoreFileUsingBM25(fileMatch *FileMatch, doc uint32, cands [
termFreqs := map[string]int{}
for _, cand := range cands {
term := string(cand.substrLowered)
termFreqs[term]++

if cand.fileName {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In text search, it's super common to "boost" matches on document title. The same thing applies here, as a filename match is strong signal. The choice of 2 is quite conservative, and means that a match on filename counts for two matches on content. I tried a few choices and didn't see evidence that a larger number would help.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat!

score.go Show resolved Hide resolved
@jtibshirani
Copy link
Member Author

This improves slightly on our "golden queries" set. Before, for the query "owner for wildcard library" we didn't find client/wildcard/OWNERS. With this change, we now find it.

Although the change doesn't make a huge difference on this eval set, I think it's worth it just for correctness/ consistency of our BM25 implementation.

@jtibshirani jtibshirani requested a review from a team April 16, 2024 20:59
@jtibshirani jtibshirani marked this pull request as ready for review April 16, 2024 20:59
termFreqs[term] += 2
} else {
termFreqs[term]++
}
}

// Compute the file length ratio. Usually the calculation would be based on terms, but using
// bytes should work fine, as we're just computing a ratio.
fileLength := float64(d.boundaries[doc+1] - d.boundaries[doc])
numFiles := len(d.boundaries)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do we have to guard against numFiles=0 too?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this should never happen -- if you had an empty shard, there's no possibility to match and score a doc?

build/scoring_test.go Show resolved Hide resolved
@@ -127,14 +130,24 @@ func (d *indexData) scoreFileUsingBM25(fileMatch *FileMatch, doc uint32, cands [
termFreqs := map[string]int{}
for _, cand := range cands {
term := string(cand.substrLowered)
termFreqs[term]++

if cand.fileName {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Neat!

eval.go Outdated Show resolved Hide resolved
eval.go Show resolved Hide resolved
@jtibshirani jtibshirani merged commit 43b9225 into main Apr 17, 2024
9 checks passed
@jtibshirani jtibshirani deleted the jtibs/filename branch April 17, 2024 16:53
Copy link
Member

@keegancsmith keegancsmith left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants