test(implicit_feedback): age-vs-alpha-drift Pearson correlation guard (#555) - #562
test(implicit_feedback): age-vs-alpha-drift Pearson correlation guard (#555)#562robotrocketscience wants to merge 1 commit into
Conversation
…#555) Synthetic 1-week workload: 200 beliefs created uniformly over 7 days, retrieval counts drawn from Poisson(lam=2) and decorrelated from age by construction. Sweeper driven end-to-end; asserts Pearson r(age, alpha_drift) in [-0.2, 0.5] -- guards against the mechanism becoming a pure clock (r>0.5) while tolerating sampling noise near zero (lower bound -0.2 ~= 3 sigma).
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ 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 selected for processing (1)
✨ 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 |
Reviewer's GuideAdds a new regression test that builds a synthetic 1-week implicit-feedback workload, runs the deferred-feedback sweeper end-to-end, and validates that the Pearson correlation between belief age and alpha drift stays within a statistically justified bound [-0.2, 0.5] to guard against the sweeper effectively becoming a time-based clock or penalizing older beliefs. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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 |
|
[claim:review:Maxwell:2026-05-10T05:21:54Z] |
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In
_build_workload, the explicit-feedback loop repeatedly callsbelief_ids.index(bid), which is O(N^2); consider building adictmappingbelief_id -> creation_offset_sonce and reusing it to avoid the repeated list search. - You have the window length expressed as
_WINDOW_SECONDS = WINDOW_DAYS * 86_400and also hardcode86_400.0in_age_days; using a single shared constant for seconds-per-day would reduce the risk of these diverging in future changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_build_workload`, the explicit-feedback loop repeatedly calls `belief_ids.index(bid)`, which is O(N^2); consider building a `dict` mapping `belief_id -> creation_offset_s` once and reusing it to avoid the repeated list search.
- You have the window length expressed as `_WINDOW_SECONDS = WINDOW_DAYS * 86_400` and also hardcode `86_400.0` in `_age_days`; using a single shared constant for seconds-per-day would reduce the risk of these diverging in future changes.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
Closing this PR per operator review. The substantive issue isn't the bound numbers — it's the choice of correlation measure. Pearson r only detects linear correlation. A non-linear age dependence — e.g. a sweeper bug that disproportionately suppresses both very-young and very-old beliefs (U-shape), or a mid-window plateau — would produce r ≈ 0 and slip past this guard entirely. The "implicit feedback becomes a clock" failure mode the issue is trying to catch is not guaranteed to be linear in age, so a Pearson-only guard is the wrong tool for the job. The test (and the issue body) need to switch to non-linear correlation measures:
Issue #555 is being reopened with a revised acceptance bullet specifying the correlation measures. The bound numbers will follow from that. Closing this PR rather than asking for changes because the rewrite is large enough that a fresh PR off updated spec is cleaner. |
|
[release:review:Maxwell:2026-05-10T05:27:01Z] |
Closes #555 — follow-up from #546 (which closed as duplicate of shipped #191).
What lands
New file:
tests/test_implicit_feedback_age_correlation.py. Single test that:T_sweep = T_end + 2·T_graceso every grace window has elapsed.scipy.stats.pearsonr(age_days, alpha − alpha_initial).Measured value at seed 42: r = -0.035 (well within the bound).
Spec deviation — please review
Issue body specifies bound
[0.0, 0.5]. This test ships[-0.2, 0.5].Rationale: the issue body also specifies "retrieval frequency must be uncorrelated with age." With a truly decorrelated workload, the true correlation is 0 and the sample r at N=200 hovers around 0 with σ ≈ 0.07 (standard error of Pearson r at this sample size). Clamping the lower bound at exactly 0.0 would produce spurious failures on roughly half of seeds (any time noise pushes r slightly negative).
The lower bound
-0.2(about 3σ below zero) catches a real "systematic anti-correlation" regression — sweeper bias that disproportionately suppresses old beliefs — without firing on sampling noise. The upper bound0.5is preserved exactly as specified — that's the actual guard for the "implicit feedback becomes a clock" regression the issue is concerned about.If the operator prefers the strict
[0.0, 0.5]bound, the test should add multi-seed averaging (e.g. mean r across 30 seeds) so the noise floor doesn't trip the lower edge — that's a more involved change. Flag here so the reviewer can choose.The test docstring includes the same explanation so a future maintainer can reason about the bound.
Out of scope
Per the issue body:
epsilon/T_grace(tuning concern, not regression guard).Notes
github/main(e45fde3). Single commit, signed.tests/test_implicit_feedback.py(29 tests) continues to pass — this PR adds coverage, doesn't replace.Summary by Sourcery
Tests: