feat(bm25): score content and anchor as two normalised BM25F fields (#1180) - #1219
Conversation
There was a problem hiding this comment.
Sorry @robotrocketscience, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
Next review available in: 52 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's GuideImplements a per-field BM25F scorer that separates content and anchor streams, adds configuration/flags and serialization support for the new mode, and introduces tests and docs to verify behavior and explain gating/bench expectations while preserving legacy behavior by default. Sequence diagram for per-field BM25F scoring path resolutionsequenceDiagram
participant Retrieval_l1_hits as _l1_hits
participant Config as resolve_bm25f_per_field / resolve_bm25_b_anchor
participant CacheFactory as _store_scoped_bm25f_cache
participant Cache as BM25IndexCache
participant Index as BM25Index
Retrieval_l1_hits->>Config: resolve_bm25f_per_field()
Config-->>Retrieval_l1_hits: per_field
Retrieval_l1_hits->>Config: resolve_bm25_b_anchor()
Config-->>Retrieval_l1_hits: b_anchor
Retrieval_l1_hits->>CacheFactory: _store_scoped_bm25f_cache(anchor_weight,k3,per_field,b_anchor)
CacheFactory->>Cache: BM25IndexCache(store,anchor_weight,k3,per_field,b_anchor)
CacheFactory->>Cache: get()
Cache->>Index: build(store,anchor_weight,k1,b,k3,per_field,b_anchor)
Retrieval_l1_hits->>Index: score(query,top_k)
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PR-size soft capThis PR is over the advisory size threshold:
Bigger PRs collide with more open work, which under the parallel-session workflow tends to produce repeated This is advisory only — nothing is blocked. If the size is intentional (large refactor, module removal, generated code), apply the |
|
[claim:review:garsecg:2026-07-30T21:05:21Z] |
Review: approved, with one claim that needs weakeningCI green. Commits atomic and signed. Discretion grep on added lines clean. The claim that ships is verifiedThe default path is what production runs, and "legacy is untouched" holds. Byte-identical, not approximately. The The per-field math checks out against Robertson/Zaragoza/Taylor. Applying
|
|
Correcting my own last line: holding I said the exactness finding was wording-and-test-robustness and did not block.
Unqualified, in a permanent user-facing record, and I have a direct What unblocks it, both small:
Everything else in my review stands — legacy path verified byte-identical to |
|
[release:review:garsecg:2026-07-30T21:09:49Z] |
|
[claim:review:Setr:2026-07-30T21:13:14Z] |
Review: approved. Prior round's two asks are now landed on the branch.The previous review approved the substance and held Independently reproduced, both findingsThe legacy path really is untouched. Built the same 300-belief corpus That is the whole safety argument for a default-off flag, so it is worth
Same numbers as the first round to three digits, from a different corpus. It I did check the one way this could have been a real difference rather than Pushed —
|
The lane concatenated each belief's incoming anchor text into its own document and normalised by the combined length — the single-field stream-replication approximation, not BM25F. Because the replicas land in the same dl, a belief's own content terms were length-penalised in proportion to how much text its citers wrote about it. Adds the Robertson/Zaragoza/Taylor (2004) form behind per_field: tf~ = SUM_f w_f*tf_f/B_f with per-stream B_f, saturated as (k1+1)*tf~/(k1+tf~). anchor_weight becomes a field weight rather than a replication count, b_anchor is a new tunable, df counts a term once across the union of the two streams, and serialisation goes to v4. The (k1+1) numerator is not in the paper's rank-equivalent presentation, which drops it as a constant factor. It is kept because it is what makes anchor_weight=0 reproduce the legacy lane's scores exactly rather than at 1/(k1+1) of them. Default off; the legacy path is untouched and byte-identical.
resolve_bm25f_per_field and resolve_bm25_b_anchor follow the existing env -> kwarg -> TOML -> default chain, and both ride the BM25IndexCache invalidation path alongside anchor_weight and k3: they are carried on the built index, so a cached index built under different values would keep scoring with the stale ones. b_anchor only participates in scoring under per_field, so it is compared for invalidation only there — flipping an inert knob must not force a rebuild. Both default to the shipped behaviour, so retrieval is unchanged.
Notes the two corrections the implementation forced on the filed issue (the missing (k1+1) numerator, and the 'unbounded boost' claim that BM25 saturation refutes), and states what the gating bench must control for: the anchor stream is sparse to absent in practice, and per-field's boost is coupled to corpus-wide anchor density, so a neutral result on a corpus with no anchors is a no-measurement rather than a refutation.
The exact `==` passed because the fixture is small enough to round identically, not because the property holds: on a 400-belief corpus all 200 probe queries disagree in the last bits at a relative delta of 2.2e-07, since legacy divides by `(tf + k1*B)` while per-field divides by `B` first and then saturates. `rel=1e-6` still fails by six orders of magnitude if the `(k1+1)` numerator is dropped, which is the property the test is for.
"Zero delta" is false as written and lands in a permanent user-facing record: the two paths agree only to float32 rounding. State the measured figures so the next person to bench the flag reads a 1e-6 delta at w_anchor=0 as rounding rather than a regression.
5ccd428 to
1f86537
Compare
|
merge-train: merged 1f86537 → |
|
[release:review:Setr:2026-07-30T21:28:53Z] |
Closes #1180.
Implements the per-field BM25F scorer behind
[retrieval] bm25f_per_field(default off), plus
bm25_b_anchoras the anchor stream'sb.What was wrong
The lane concatenates each belief's incoming anchor text into its own
document and normalises by the combined length. Because the replicas
land in the same
dl, a belief's own content terms are length-penalisedin proportion to how much text its citers wrote about it.
Measured on a corpus where two beliefs are identical in everything BM25
can see except that one is cited, and the anchor text never mentions the
query term:
A cited belief is demoted to roughly a quarter of an identical uncited
one, hardest at the sparse densities production actually runs at, for
something no part of the belief itself did.
What changed
tf~ = Σ_f w_f·tf_f/B_fwith per-streamB_f = (1-b_f) + b_f·dl_f/avgdl_f,saturated as
(k1+1)·tf~/(k1+tf~).anchor_weightbecomes a field weightrather than a replication count;
dfcounts a term once across the unionof the two streams; serialisation goes to v4.
Default off because this replaces the functional form rather than
re-parameterising it — the saturation denominator becomes the constant
k1instead oftf + k1·B, so no choice of constants makes on and offagree once an anchor stream exists. There is no parity test that could
gate the flip, only a bench. The legacy path is untouched and verified
byte-identical to
mainacross a query/weight grid.Two corrections to the issue
The stated formula breaks the issue's own acceptance criterion. It
gives
score_t = idf·tf~/(k1+tf~), dropping the(k1+1)numerator asRobertson's rank-equivalent presentation does. That leaves every score a
factor of 2.5 below the current lane at the shipped
k1 = 1.5, sow_anchor = 0would not "recover standard BM25 byte-exact". Keepingthe numerator makes it exact —
|delta| = 0on every probe, and thetest asserts full score equality, not just ordering.
"Unbounded" is false. The issue rejects the simpler content-only-
dlfix on the grounds that it "trades a bounded penalty for an unbounded
boost". BM25 saturation caps every variant at
idf·(k1+1); that fix'sboost converges to exactly 1.75x, measured out to a term frequency of
3e9. It is still the wrong instrument — the boost is unearned, paying
no length normalisation at all — but the conclusion needed a different
argument, and the acceptance criteria as written encode the wrong reason.
What the gating bench must control for
The anchor stream is sparse to absent in practice. The one real store
available for measurement holds 16,454 beliefs and no edges at all;
60 realistic turns through the production
ingest_turnpath produced1 anchored belief in 15, with a one-token anchor.
Per-field's boost is strongly coupled to corpus-wide anchor density
(1.08x at 1% anchored, 1.91x at 100%), because
avgdl_anchoraveragesover every document including the many with no anchor text. So a
benchmark over a corpus with little anchor text will report a near-zero
delta that says nothing about the scoring change — the #1160 failure
mode. Any bench of this flag should publish, alongside the delta:
A neutral result on a corpus with no anchors is a no-measurement, not
a refutation. The demotion fix, unlike the boost, is density-independent
— which is the argument for the change surviving a weak bench.
Acceptance criteria
tf_content/tf_anchorCSR matrices,dl_*,avgdl_*tf~;w_anchortakes overanchor_weight's rolew_anchor = 0recovers standard BM25 over content alone, exactlyb_anchortunable; default 0.75, justified inbm25.pyand CONFIGb_anchordefaults to the content stream'sbdeliberately: the fieldsplit already changes the functional form, so giving the anchor stream a
different
bat the same time would confound "fields were separated"with "the anchor stream is normalised differently". 0.75 also reproduces
the issue's own worked figures (0.5714 / 0.7835) exactly.
Tests
tests/test_bm25_per_field.py, 17 cases.test_legacy_demotes_cited_belief_below_uncitedis the distinguishingassert for the file — it pins the defect on the legacy path, so the
per-field assertions cannot pass vacuously if the branch is later
short-circuited or the flag stops reaching
build().Full suite: 6448 passed, 69 skipped, 71 xfailed.
Summary by Sourcery
Introduce a bench-gated per-field BM25F mode that scores content and anchor text as two separately normalised fields with configurable anchor length-normalisation, updating index storage, retrieval configuration, and tests while preserving legacy behaviour by default.
New Features:
Bug Fixes:
Enhancements:
Documentation:
Tests: