feat(bm25): Porter stemming on the BM25F lane (#154) - #428
Merged
Conversation
Adds `snowballstemmer` runtime dep + `tokenize_stemmed()` helper. `BM25Index.build` and `BM25Index.score` now stem at index/query time so the BM25F lane has FTS5-equivalent matching behavior. Without this, switching the L1 lane to BM25F by default would silently regress queries like q="banana" against content "bananas" — FTS5's Porter stemming caught those for free; stemless BM25F missed them. `tokenize()` (without stem) stays for callers like `relationship_detector` that depend on word-form-preserving tokens to match against unstemmed quantifier vocabulary like "always" / "rarely". Splitting the helpers keeps each call site semantically correct. Bench evidence: re-running the per-flag NDCG@k harness on the v0.1 retrieve_uplift fixture under stemming raises use_bm25f_anchors uplift from +0.6010 to +0.6650 (other four flags unchanged at 0.0000). Stemming unblocks the v1.7 default-on flip for use_bm25f_anchors per #154.
Reviewer's GuideAdds Porter stemming to the BM25F lane via a new tokenize_stemmed() helper and a snowballstemmer runtime dependency, and switches BM25Index.build/score to use it while preserving the existing unstemmed tokenize() behavior for non-BM25 callers. Sequence diagram for BM25F query scoring with Porter stemmingsequenceDiagram
actor User
participant BM25Index
participant bm25_module
participant tokenize_stemmed
participant PorterStemmer
User->>BM25Index: score(query)
BM25Index->>bm25_module: tokenize_stemmed(query)
bm25_module->>tokenize_stemmed: call with text
tokenize_stemmed->>tokenize_stemmed: lowercase and find tokens
loop for each token
tokenize_stemmed->>PorterStemmer: stemWord(token)
PorterStemmer-->>tokenize_stemmed: stemmed_token
end
tokenize_stemmed-->>bm25_module: list of stemmed tokens
bm25_module-->>BM25Index: q_tokens
BM25Index->>BM25Index: compute idf weighted tf for q_tokens
BM25Index-->>User: ranked_results
Updated class diagram for BM25 tokenization and BM25Index build/scoreclassDiagram
class bm25_module {
+regex _TOKEN_PATTERN
+stemmer _PORTER_STEMMER
+list~str~ tokenize(text)
+list~str~ tokenize_stemmed(text)
}
class PorterStemmer {
+list~str~ stemWord(word)
}
class BM25Index {
+sp.csr_matrix tf
+np.ndarray idf
+int n_docs
+float k1
+float b
+score(query) list~tuple~
}
class BM25_build_function {
+BM25Index build(beliefs, contents, incoming, anchor_weight)
}
bm25_module --> PorterStemmer : uses
bm25_module --> BM25_build_function : defines
bm25_module --> BM25Index : defines
BM25_build_function --> BM25Index : constructs
BM25_build_function --> bm25_module : calls tokenize_stemmed
BM25Index --> bm25_module : score calls tokenize_stemmed
bm25_module ..> bm25_module : tokenize used by non_BM25_callers
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider updating the
BM25Index-related docstrings/comments that describe the canonical tokenisation to explicitly mentiontokenize_stemmed()(and the stemming behavior) so future readers don’t assume BM25 still uses the unstemmedtokenize()path. - To avoid the tokenisation behavior drifting over time, you could have
tokenize_stemmed()calltokenize()and apply the Porter stemmer to its output, instead of duplicating the_TOKEN_PATTERN+lower()logic in two places.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider updating the `BM25Index`-related docstrings/comments that describe the canonical tokenisation to explicitly mention `tokenize_stemmed()` (and the stemming behavior) so future readers don’t assume BM25 still uses the unstemmed `tokenize()` path.
- To avoid the tokenisation behavior drifting over time, you could have `tokenize_stemmed()` call `tokenize()` and apply the Porter stemmer to its output, instead of duplicating the `_TOKEN_PATTERN` + `lower()` logic in two places.
## Individual Comments
### Comment 1
<location path="src/aelfrice/bm25.py" line_range="109-128" />
<code_context>
return [m.group(0).lower() for m in _TOKEN_PATTERN.finditer(text)]
+def tokenize_stemmed(text: str) -> list[str]:
+ """Lowercase + Unicode-word tokenisation + Porter stemming.
+
+ Used by `BM25Index.build` and `BM25Index.score` so the BM25F
+ lane has FTS5-equivalent stemming. SQLite FTS5 uses Porter by
+ default; without stemming on the BM25F path,
+ `q="banana"` against content `"bananas"` would miss matches that
+ the legacy FTS5 lane catches. Added at v1.7.0 (#154) when the
+ default-on flip was prepared.
+
+ Non-BM25 callers (relationship_detector, scoring helpers, etc.)
+ that depend on word-form-preserving tokens should keep using
+ `tokenize()`; stemming is BM25-specific.
+ """
+ if not text:
+ return []
+ return [
+ _PORTER_STEMMER.stemWord(m.group(0).lower())
+ for m in _TOKEN_PATTERN.finditer(text)
+ ]
</code_context>
<issue_to_address>
**suggestion:** Avoid duplicating tokenisation logic by reusing `tokenize()` inside `tokenize_stemmed()`.
Both functions reapply `_TOKEN_PATTERN` and `.lower()` independently. To keep behavior aligned and avoid divergence, have `tokenize_stemmed()` call `tokenize()` and stem the resulting tokens:
```python
base_tokens = tokenize(text)
return [_PORTER_STEMMER.stemWord(tok) for tok in base_tokens]
```
This ensures any future tokenization changes automatically apply to the stemmed path as well.
```suggestion
def tokenize_stemmed(text: str) -> list[str]:
"""Lowercase + Unicode-word tokenisation + Porter stemming.
Used by `BM25Index.build` and `BM25Index.score` so the BM25F
lane has FTS5-equivalent stemming. SQLite FTS5 uses Porter by
default; without stemming on the BM25F path,
`q="banana"` against content `"bananas"` would miss matches that
the legacy FTS5 lane catches. Added at v1.7.0 (#154) when the
default-on flip was prepared.
Non-BM25 callers (relationship_detector, scoring helpers, etc.)
that depend on word-form-preserving tokens should keep using
`tokenize()`; stemming is BM25-specific.
"""
base_tokens = tokenize(text)
return [_PORTER_STEMMER.stemWord(tok) for tok in base_tokens]
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+109
to
+128
| def tokenize_stemmed(text: str) -> list[str]: | ||
| """Lowercase + Unicode-word tokenisation + Porter stemming. | ||
|
|
||
| Used by `BM25Index.build` and `BM25Index.score` so the BM25F | ||
| lane has FTS5-equivalent stemming. SQLite FTS5 uses Porter by | ||
| default; without stemming on the BM25F path, | ||
| `q="banana"` against content `"bananas"` would miss matches that | ||
| the legacy FTS5 lane catches. Added at v1.7.0 (#154) when the | ||
| default-on flip was prepared. | ||
|
|
||
| Non-BM25 callers (relationship_detector, scoring helpers, etc.) | ||
| that depend on word-form-preserving tokens should keep using | ||
| `tokenize()`; stemming is BM25-specific. | ||
| """ | ||
| if not text: | ||
| return [] | ||
| return [ | ||
| _PORTER_STEMMER.stemWord(m.group(0).lower()) | ||
| for m in _TOKEN_PATTERN.finditer(text) | ||
| ] |
There was a problem hiding this comment.
suggestion: Avoid duplicating tokenisation logic by reusing tokenize() inside tokenize_stemmed().
Both functions reapply _TOKEN_PATTERN and .lower() independently. To keep behavior aligned and avoid divergence, have tokenize_stemmed() call tokenize() and stem the resulting tokens:
base_tokens = tokenize(text)
return [_PORTER_STEMMER.stemWord(tok) for tok in base_tokens]This ensures any future tokenization changes automatically apply to the stemmed path as well.
Suggested change
| def tokenize_stemmed(text: str) -> list[str]: | |
| """Lowercase + Unicode-word tokenisation + Porter stemming. | |
| Used by `BM25Index.build` and `BM25Index.score` so the BM25F | |
| lane has FTS5-equivalent stemming. SQLite FTS5 uses Porter by | |
| default; without stemming on the BM25F path, | |
| `q="banana"` against content `"bananas"` would miss matches that | |
| the legacy FTS5 lane catches. Added at v1.7.0 (#154) when the | |
| default-on flip was prepared. | |
| Non-BM25 callers (relationship_detector, scoring helpers, etc.) | |
| that depend on word-form-preserving tokens should keep using | |
| `tokenize()`; stemming is BM25-specific. | |
| """ | |
| if not text: | |
| return [] | |
| return [ | |
| _PORTER_STEMMER.stemWord(m.group(0).lower()) | |
| for m in _TOKEN_PATTERN.finditer(text) | |
| ] | |
| def tokenize_stemmed(text: str) -> list[str]: | |
| """Lowercase + Unicode-word tokenisation + Porter stemming. | |
| Used by `BM25Index.build` and `BM25Index.score` so the BM25F | |
| lane has FTS5-equivalent stemming. SQLite FTS5 uses Porter by | |
| default; without stemming on the BM25F path, | |
| `q="banana"` against content `"bananas"` would miss matches that | |
| the legacy FTS5 lane catches. Added at v1.7.0 (#154) when the | |
| default-on flip was prepared. | |
| Non-BM25 callers (relationship_detector, scoring helpers, etc.) | |
| that depend on word-form-preserving tokens should keep using | |
| `tokenize()`; stemming is BM25-specific. | |
| """ | |
| base_tokens = tokenize(text) | |
| return [_PORTER_STEMMER.stemWord(tok) for tok in base_tokens] |
4 tasks
robotrocketscience
added a commit
that referenced
this pull request
May 5, 2026
Updates the v1.7 row to reflect the post-stemming bench result and the actual default-on flip: - BM25F anchor-text retrieval (#148) default-on at v1.7.0 per #154 bench evidence: +0.6650 NDCG@k uplift on the v0.1 retrieve_uplift fixture under Porter stemming. PR #428 added the stemmer; PR #430 flipped the default; bench-gate test_retrieve_per_flag_no_regression PASS. - Other v1.7 components (use_signed_laplacian, use_heat_kernel, use_hrr_structural) remain opt-in — placeholder lanes pending wiring into retrieve(). The v1.7 wave is shipped; the remaining-flags flip waits on those lanes landing. - v2.0 row drops the "default-on flip is a prereq" note since v1.7 is now shipped. Replaces the prior intermediate framing ("shipped (opt-in)"; deferred default-on flip).
2 tasks
Collaborator
Author
|
[claim:review:Kulili:2026-05-05T16:17:22Z] |
robotrocketscience
added a commit
that referenced
this pull request
May 5, 2026
Updates the v1.7 row to reflect the post-stemming bench result and the actual default-on flip: - BM25F anchor-text retrieval (#148) default-on at v1.7.0 per #154 bench evidence: +0.6650 NDCG@k uplift on the v0.1 retrieve_uplift fixture under Porter stemming. PR #428 added the stemmer; PR #430 flipped the default; bench-gate test_retrieve_per_flag_no_regression PASS. - Other v1.7 components (use_signed_laplacian, use_heat_kernel, use_hrr_structural) remain opt-in — placeholder lanes pending wiring into retrieve(). The v1.7 wave is shipped; the remaining-flags flip waits on those lanes landing. - v2.0 row drops the "default-on flip is a prereq" note since v1.7 is now shipped. Replaces the prior intermediate framing ("shipped (opt-in)"; deferred default-on flip).
4 tasks
robotrocketscience
added a commit
that referenced
this pull request
May 5, 2026
Updates the v1.7 row to reflect the post-stemming bench result and the actual default-on flip: - BM25F anchor-text retrieval (#148) default-on at v1.7.0 per #154 bench evidence: +0.6650 NDCG@k uplift on the v0.1 retrieve_uplift fixture under Porter stemming. PR #428 added the stemmer; PR #430 flipped the default; bench-gate test_retrieve_per_flag_no_regression PASS. - Other v1.7 components (use_signed_laplacian, use_heat_kernel, use_hrr_structural) remain opt-in — placeholder lanes pending wiring into retrieve(). The v1.7 wave is shipped; the remaining-flags flip waits on those lanes landing. - v2.0 row drops the "default-on flip is a prereq" note since v1.7 is now shipped. Replaces the prior intermediate framing ("shipped (opt-in)"; deferred default-on flip).
robotrocketscience
added a commit
that referenced
this pull request
May 5, 2026
Updates the v1.7 row to reflect the post-stemming bench result and the actual default-on flip: - BM25F anchor-text retrieval (#148) default-on at v1.7.0 per #154 bench evidence: +0.6650 NDCG@k uplift on the v0.1 retrieve_uplift fixture under Porter stemming. PR #428 added the stemmer; PR #430 flipped the default; bench-gate test_retrieve_per_flag_no_regression PASS. - Other v1.7 components (use_signed_laplacian, use_heat_kernel, use_hrr_structural) remain opt-in — placeholder lanes pending wiring into retrieve(). The v1.7 wave is shipped; the remaining-flags flip waits on those lanes landing. - v2.0 row drops the "default-on flip is a prereq" note since v1.7 is now shipped. Replaces the prior intermediate framing ("shipped (opt-in)"; deferred default-on flip).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the stemming-gap blocker on #154's default-on flip. Surfaced at #154 comment 4380887104.
What ships
snowballstemmer>=2.2added as a runtime dep. Pure-Python, ~150 KB, deterministic for the English Porter algorithm.tokenize_stemmed()helper inaelfrice.bm25— lowercase + Unicode-word splitting + Porter stemming.BM25Index.buildandBM25Index.scoreswitched to it so BM25F has FTS5-equivalent matching.tokenize()(existing) stays unstemmed —aelfrice.relationship_detectorand any other caller that depends on word-form-preserving tokens (e.g. matching"always"/"never"/"rarely"against unstemmedQUANT_AXISkeys) keeps the old contract.Why this is needed
Without stemming, BM25F missed natural-language queries that FTS5's Porter stemmer caught for free:
Switching the L1 lane to BM25F by default would silently break user queries until the stemmer was added. With this PR, both lanes match.
Bench evidence
Re-ran
python -m tests.retrieve_uplift_runneragainst the v0.1 lab corpus under stemming:use_bm25f_anchorsuse_signed_laplacianuse_heat_kerneluse_posterior_rankinguse_hrr_structuraluse_bm25f_anchorsuplift improved by +0.064 NDCG@k (more rows match because stem-divergent queries now hit). The other four flags are unchanged — placeholder flags or fixture-too-small per the prior #154 comment 4380842909.Out of scope (subsequent #154 tasks)
use_bm25f_anchorsuplift went UP, not DOWN.Test plan
uv run pytest --ignore=tests/bench_gate -q— 2456 passed, 23 skipped.AELFRICE_CORPUS_ROOT=... uv run pytest tests/bench_gate/test_retrieve_uplift.py— PASS, no per-flag regressions.q="banana"against content"bananas"now returns['F1']under BM25F (was[]).relationship_detectorquantifier-axis tests still pass — confirmedtokenize()retains unstemmed behavior for non-BM25 callers.Summary by Sourcery
Add Porter-stemmed tokenization to the BM25F path for parity with FTS5 and improved recall.
New Features:
Enhancements:
Build: