feat(query_understanding): R1 entity-expand + R3 IDF-clip modules (#291 PR-1) - #368
Merged
Merged
Conversation
Reviewer's GuideIntroduces a new aelfrice.query_understanding package implementing the R1 entity-expansion and R3 IDF-clip query rewriting transforms, plus per-store IDF quantile threshold computation, and wires them into a public API with dedicated unit tests; no existing call sites or BM25 plumbing are changed yet. Class diagram for new query_understanding package functions and constantsclassDiagram
class entity_expand {
<<module>>
+int DEFAULT_QF_MULTIPLIER
+list~str~ expand_with_capitalised_entities(raw_query: str, base_terms: list~str~, qf_multiplier: int)
}
class idf_clip {
<<module>>
+float DEFAULT_LOW_QUANTILE
+float DEFAULT_HIGH_QUANTILE
+int DEFAULT_BOOST_QF
+tuple~float,float~ compute_idf_quantile_thresholds(idf: np.ndarray, low_quantile: float, high_quantile: float)
+list~str~ clip_with_quantile_thresholds(terms: list~str~, vocabulary: dict~str,int~, idf: np.ndarray, low_threshold: float, high_threshold: float, boost_qf: int)
}
class query_understanding_package {
<<package API>>
+int DEFAULT_QF_MULTIPLIER
+int DEFAULT_BOOST_QF
+float DEFAULT_LOW_QUANTILE
+float DEFAULT_HIGH_QUANTILE
+list~str~ expand_with_capitalised_entities(raw_query: str, base_terms: list~str~, qf_multiplier: int)
+tuple~float,float~ compute_idf_quantile_thresholds(idf: np.ndarray, low_quantile: float, high_quantile: float)
+list~str~ clip_with_quantile_thresholds(terms: list~str~, vocabulary: dict~str,int~, idf: np.ndarray, low_threshold: float, high_threshold: float, boost_qf: int)
}
query_understanding_package ..> entity_expand : reexports
query_understanding_package ..> idf_clip : reexports
Flow diagram for R1 entity expansion and R3 IDF clip pipelineflowchart TD
subgraph Store_side[BeliefStore / BM25Index side]
idf_vec["BM25Index.idf (np.ndarray)"]
compute_thresh["compute_idf_quantile_thresholds(idf, low_quantile, high_quantile)"]
low_t["low_threshold: float"]
high_t["high_threshold: float"]
idf_vec --> compute_thresh
compute_thresh --> low_t
compute_thresh --> high_t
end
subgraph Query_side[Per-query rewriting side]
raw_q["raw_query: str"]
base_terms["base_terms: list[str]"]
entity_expand["expand_with_capitalised_entities(raw_query, base_terms, qf_multiplier)"]
expanded_terms["terms: list[str]"]
raw_q --> entity_expand
base_terms --> entity_expand
entity_expand --> expanded_terms
vocab["vocabulary: dict[str,int]"]
idf_vec2["idf: np.ndarray"]
idf_vec2 --- idf_vec
clip["clip_with_quantile_thresholds(terms, vocabulary, idf, low_threshold, high_threshold, boost_qf)"]
final_terms["final_terms: list[str]"]
expanded_terms --> clip
vocab --> clip
idf_vec2 --> clip
low_t --> clip
high_t --> clip
clip --> final_terms
end
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
📝 WalkthroughWalkthroughA new ChangesQuery Understanding Stack
Sequence DiagramsequenceDiagram
participant Client
participant R1 as Entity Expansion<br/>(R1)
participant R3 as IDF Clipping<br/>(R3)
participant Result as Rewritten Query
Client->>R1: raw_query + base_terms<br/>(with qf_multiplier)
R1->>R1: Scan for capitalized tokens
R1->>R1: Append lowercased copies
R1->>R3: expanded_terms
R3->>R3: Look up IDF for each term
R3->>R3: Compare vs quantile thresholds
alt IDF < low_threshold
R3->>R3: Drop term
else low_threshold ≤ IDF ≤ high_threshold
R3->>R3: Keep once
else IDF > high_threshold
R3->>R3: Emit boost_qf copies
end
R3->>Result: Final clipped/boosted terms
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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 left some high level feedback:
- In
clip_with_quantile_thresholds, consider validating that each vocabulary index is within the bounds of theidfarray (or thatidf.sizematches the vocabulary’s max index) to avoid unexpectedIndexErrorwhen the BM25 index and vocabulary get out of sync. - For
compute_idf_quantile_thresholds, you may want to explicitly document or guard against the case whereidfcontains NaNs/inf values, sincenp.quantilecan propagate them and silently produce thresholds that make the clip effectively degenerate.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `clip_with_quantile_thresholds`, consider validating that each vocabulary index is within the bounds of the `idf` array (or that `idf.size` matches the vocabulary’s max index) to avoid unexpected `IndexError` when the BM25 index and vocabulary get out of sync.
- For `compute_idf_quantile_thresholds`, you may want to explicitly document or guard against the case where `idf` contains NaNs/inf values, since `np.quantile` can propagate them and silently produce thresholds that make the clip effectively degenerate.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
This PR is now behind Auto-rebase was removed because the bot has no signing key; rebasing as the bot strips author signatures and the |
Collaborator
Author
|
[claim:review:Kulili:2026-05-03T03:53:46Z] |
Collaborator
Author
|
[claim:review:Gylf:2026-05-03T03:53:54Z] |
Collaborator
Author
|
[release:review:Gylf:2026-05-03T03:53:59Z] |
… PR-1) Adds aelfrice.query_understanding package with the two deterministic query-rewrite transforms ratified for #291: - R1 capitalised-token expansion (boost via integer-qf duplication) - R3 IDF clip with per-store quantile-derived thresholds Per-store quantile threshold computation lives alongside the clip itself so callers can cache the (low, high) tuple on a BeliefStore once per BM25 index build. PR-1 lands the modules + 24 unit tests; no call-site change. The context_rebuilder integration lands in PR-2 behind the query_strategy = "legacy-bm25" setting (#291 rollout step 2).
robotrocketscience
force-pushed
the
feat/issue-291-pr1-query-understanding
branch
from
May 3, 2026 03:55
0181b13 to
f52b4d3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/aelfrice/query_understanding/idf_clip.py (1)
73-116: ⚡ Quick winAdd a
low_threshold <= high_thresholdguard to prevent silent mis-behaviour on inverted thresholds.
clip_with_quantile_thresholdsaccepts two independentfloatparameters; if a caller accidentally passes them in the wrong order, the function silently drops everything that would normally be mid-band and boosts the wrong terms—no error, no diagnostic.compute_idf_quantile_thresholdsalways returns a correctly ordered pair, but future direct callers (e.g. cache-warmed thresholds stored separately) have no protection.🛡️ Proposed guard
if boost_qf < 1: raise ValueError(f"boost_qf must be >= 1, got {boost_qf}") + if low_threshold > high_threshold: + raise ValueError( + f"low_threshold ({low_threshold}) must be <= high_threshold ({high_threshold})" + ) out: list[str] = []🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/aelfrice/query_understanding/idf_clip.py` around lines 73 - 116, Add a guard in clip_with_quantile_thresholds to detect inverted thresholds and raise a clear error instead of silently misbehaving: check that low_threshold <= high_threshold at the start of the function (after validating boost_qf) and raise a ValueError with a descriptive message if not, referencing the provided low_threshold and high_threshold values so callers get immediate feedback; this change is to the clip_with_quantile_thresholds function and should not affect compute_idf_quantile_thresholds which already returns ordered pairs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/aelfrice/query_understanding/idf_clip.py`:
- Around line 73-116: Add a guard in clip_with_quantile_thresholds to detect
inverted thresholds and raise a clear error instead of silently misbehaving:
check that low_threshold <= high_threshold at the start of the function (after
validating boost_qf) and raise a ValueError with a descriptive message if not,
referencing the provided low_threshold and high_threshold values so callers get
immediate feedback; this change is to the clip_with_quantile_thresholds function
and should not affect compute_idf_quantile_thresholds which already returns
ordered pairs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 93a87d15-68c3-42be-b3c9-fa74a21ffdff
📒 Files selected for processing (4)
src/aelfrice/query_understanding/__init__.pysrc/aelfrice/query_understanding/entity_expand.pysrc/aelfrice/query_understanding/idf_clip.pytests/test_query_understanding.py
Collaborator
Author
|
[release:review:Kulili:2026-05-03T04:04:23Z] |
yoshi280
removed
attn:review
4 tasks
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.
First of four atomic PRs implementing the ratified scope of #291 (R1 + R3 query-understanding stack).
What
New
aelfrice.query_understandingpackage with two pure transforms and a per-store quantile helper:expand_with_capitalised_entities(raw_query, base_terms, *, qf_multiplier=2)— R1: detect capitalised tokens, lowercase, appendqf_multipliercopies. Pure regex; sub-microsecond.compute_idf_quantile_thresholds(idf, low_q=0.25, high_q=0.75)— derive per-store low/high IDF cutoffs from aBM25Index.idfvector. Replaces the synthetic-tuned (1.5, 2.5) constants from the lab R3.5 audit, which are inert at live IDF medians of 7.5–9.5 (R5 prereq survey, Rebuild redesign: query understanding (entity extraction, intent, term-weighting) #291 body).clip_with_quantile_thresholds(terms, vocabulary, idf, low, high, *, boost_qf=2)— R3: drop terms with IDF strictly belowlow, duplicate (boost_qfcopies) terms strictly abovehigh, mid-band emits once.24 unit tests covering both transforms, edge cases (empty vectors, OOV terms, boundary IDF, invalid args), and one R1→R3 composition smoke test.
What this PR does not change
No call site is touched.
context_rebuilder.py,retrieval.py,bm25.py, andstore.pyare unchanged. The integration behindquery_strategy = "legacy-bm25"lands in PR-2 (#291 rollout step 2).Verification
uv run pytest tests/test_query_understanding.py -q→ 24 passed.uv run pytest tests/ --ignore=tests/regression --ignore=tests/bench_gate --ignore=tests/e2e -q→ 2067 passed, 14 skipped (no regression).git diff github/main...HEAD: clean.Why per-store quantiles, not synthetic constants
The lab R5 survey (see #291 body, "Calibration evidence") found live store IDF medians at 7.5–9.5 vs the synthetic 2.85. The R3.5 tuned constants (1.5, 2.5) sit far below the live distribution; the boost rule would fire on essentially every term, which is operationally a noop. Per-store quantiles derived from the live IDF distribution are the architectural fix and are the only design that survives the synthetic→live shape divergence.
Closes / refs
Test plan
Summary by Sourcery
Introduce a new query_understanding package providing R1 entity expansion and R3 IDF clipping building blocks, along with tests, without wiring them into existing query pipelines yet.
New Features:
Tests:
Summary by CodeRabbit
New Features
Tests