test(implicit_feedback): nonlinear age-vs-alpha-drift correlation guard (#555) - #566
Merged
robotrocketscience merged 1 commit intoMay 10, 2026
Merged
Conversation
📝 WalkthroughWalkthroughA new regression test module guards against age-correlated alpha drift in the deferred-feedback sweeper. It generates a synthetic 1-week workload with varying belief ages and exposure frequencies, runs the sweeper, computes Chatterjee's xi and distance correlation between age and alpha drift, and asserts both remain below calibrated thresholds. ChangesDeferred feedback age-correlation regression test
🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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. Comment |
Reviewer's GuideAdds a regression test that builds a synthetic implicit-feedback workload and uses Chatterjee’s xi and distance correlation to guard against nonlinear age-vs-alpha-drift correlations in the deferred-feedback sweeper. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
robotrocketscience
added
attn:review
![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
_build_workload, thebelief_ids.index(bid)lookup inside the explicit-feedback loop makes that section O(n^2); consider tracking creation offsets in a dict keyed by belief id (or iterating with indices) to keep this O(n). - The custom
_poisson_sampleimplementation is correct but adds complexity given numpy is already a dependency; consider usingnp.random.poissonwith an RNG seeded fromRNG_SEEDfor simpler, more idiomatic sampling and easier reproducibility control.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `_build_workload`, the `belief_ids.index(bid)` lookup inside the explicit-feedback loop makes that section O(n^2); consider tracking creation offsets in a dict keyed by belief id (or iterating with indices) to keep this O(n).
- The custom `_poisson_sample` implementation is correct but adds complexity given numpy is already a dependency; consider using `np.random.poisson` with an RNG seeded from `RNG_SEED` for simpler, more idiomatic sampling and easier reproducibility control.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 |
Owner
Author
|
[claim:review:maxwell:2026-05-10T05:45:33Z] |
Owner
Author
|
[release:review:maxwell:2026-05-10T05:45:55Z] |
Owner
Author
|
[claim:review:leibniz:2026-05-10T05:47:44Z] |
robotrocketscience
force-pushed
the
test/issue-555-nonlinear-correlation-guard
branch
from
May 10, 2026 05:51
58d09ae to
6297d2c
Compare
Owner
Author
|
[claim:review:einstein:2026-05-10T05:53:03Z] |
Owner
Author
|
[release:review:einstein:2026-05-10T05:53:07Z] |
…rd (#555) Replace Pearson-r bound with two measures that catch non-linear "becomes a clock" failure modes (U-shape, plateau, threshold) a linear guard misses: - Chatterjee's xi (JASA 2021) — detects monotone + non-monotone dependence. - Distance correlation (Szekely-Rizzo 2007) — detects *any* dependence. Both are implemented directly from the paper definitions using numpy (~10 and ~30 lines respectively) to avoid heavy native deps (numba/llvmlite). Thresholds calibrated across 30 seeds at N=200 as 99th-percentile + 0.05: T_XI = 0.1813 (raw p99 = 0.1313) T_DCOR = 0.2433 (raw p99 = 0.1933) Seed 42 (asserted run): xi = -0.0335, dCor = 0.1012 — both well below threshold.
robotrocketscience
force-pushed
the
test/issue-555-nonlinear-correlation-guard
branch
from
May 10, 2026 05:54
6297d2c to
bfc7d23
Compare
Owner
Author
|
[claim:review:einstein:2026-05-10T05:56:54Z] |
Owner
Author
|
[release:review:einstein:2026-05-10T05:56:59Z] |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_implicit_feedback_age_correlation.py`:
- Around line 243-275: The explicit-feedback selection and timing must be tied
to actual enqueued retrieval exposures: record each enqueued_dt when calling
enqueue_retrieval_exposures (e.g., store per-bid lists like
enqueued_times[bid].append(enqueued_dt]) and choose explicit_targets from only
those belief_ids with at least one enqueued exposure (i.e., retrieval_counts[i]
> 0 or enqueued_times[bid] non-empty). For each chosen bid, pick one of its
recorded enqueued_dt values and set fb_dt to a time inside that exposure's grace
window (e.g., enqueued_dt + a random offset in [0, T_GRACE) or otherwise ensure
fb_dt ∈ [enqueued_dt, enqueued_dt + T_GRACE)), then call
store.insert_feedback_event with created_at=_fmt(fb_dt) so the explicit feedback
will actually cancel a deferred implicit row (references: retrieval_counts,
enqueue_retrieval_exposures, enqueued_dt variable, explicit_targets,
store.insert_feedback_event).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 97940c1d-9529-4735-845c-9047608d7243
📒 Files selected for processing (1)
tests/test_implicit_feedback_age_correlation.py
Comment on lines
+243
to
+275
| # Retrieval counts drawn from Poisson(POISSON_LAMBDA), capped, and | ||
| # scheduled at random times within each belief's lifetime (decorrelated | ||
| # from age by construction: same lambda regardless of creation order). | ||
| retrieval_counts = [ | ||
| min(RETRIEVAL_CAP, _poisson_sample(rng, POISSON_LAMBDA)) | ||
| for _ in range(N_BELIEFS) | ||
| ] | ||
|
|
||
| for i, bid in enumerate(belief_ids): | ||
| creation_offset_s = creation_offsets_s[i] | ||
| lifetime_s = _WINDOW_SECONDS - creation_offset_s | ||
| for _ in range(retrieval_counts[i]): | ||
| eligible_s = max(0, lifetime_s - T_GRACE) | ||
| offset_in_lifetime = rng.randint(0, max(0, eligible_s)) | ||
| enqueued_dt = _EPOCH + timedelta( | ||
| seconds=creation_offset_s + offset_in_lifetime | ||
| ) | ||
| enqueue_retrieval_exposures(store, [bid], now=_fmt(enqueued_dt)) | ||
|
|
||
| # Explicit feedback on a fraction of beliefs cancels their implicit rows. | ||
| n_explicit = int(N_BELIEFS * EXPLICIT_FEEDBACK_FRACTION) | ||
| explicit_targets = rng.sample(belief_ids, n_explicit) | ||
| for bid in explicit_targets: | ||
| idx = belief_ids.index(bid) | ||
| creation_offset_s = creation_offsets_s[idx] | ||
| fb_offset_s = creation_offset_s + (_WINDOW_SECONDS - creation_offset_s) // 2 | ||
| fb_dt = _EPOCH + timedelta(seconds=fb_offset_s) | ||
| store.insert_feedback_event( | ||
| bid, | ||
| valence=1.0, | ||
| source="user", | ||
| created_at=_fmt(fb_dt), | ||
| ) |
There was a problem hiding this comment.
Bind explicit feedback to an actual grace window.
This workload claims that ~15% of beliefs exercise the explicit-cancellation path, but Line 264 can pick beliefs with zero retrievals, and Lines 266-269 place feedback at the lifetime midpoint instead of relative to an enqueued exposure. Most of those cases therefore won't cancel any deferred row, so the test barely covers the behavior it says it's guarding.
💡 Proposed fix
+ retrieval_times_by_belief: dict[str, list[datetime]] = {}
for i, bid in enumerate(belief_ids):
creation_offset_s = creation_offsets_s[i]
lifetime_s = _WINDOW_SECONDS - creation_offset_s
+ retrieval_times: list[datetime] = []
for _ in range(retrieval_counts[i]):
eligible_s = max(0, lifetime_s - T_GRACE)
offset_in_lifetime = rng.randint(0, max(0, eligible_s))
enqueued_dt = _EPOCH + timedelta(
seconds=creation_offset_s + offset_in_lifetime
)
enqueue_retrieval_exposures(store, [bid], now=_fmt(enqueued_dt))
+ retrieval_times.append(enqueued_dt)
+ if retrieval_times:
+ retrieval_times_by_belief[bid] = retrieval_times
# Explicit feedback on a fraction of beliefs cancels their implicit rows.
n_explicit = int(N_BELIEFS * EXPLICIT_FEEDBACK_FRACTION)
- explicit_targets = rng.sample(belief_ids, n_explicit)
+ explicit_targets = rng.sample(
+ list(retrieval_times_by_belief),
+ min(n_explicit, len(retrieval_times_by_belief)),
+ )
for bid in explicit_targets:
- idx = belief_ids.index(bid)
- creation_offset_s = creation_offsets_s[idx]
- fb_offset_s = creation_offset_s + (_WINDOW_SECONDS - creation_offset_s) // 2
- fb_dt = _EPOCH + timedelta(seconds=fb_offset_s)
+ enqueued_dt = rng.choice(retrieval_times_by_belief[bid])
+ fb_dt = enqueued_dt + timedelta(seconds=rng.randint(1, T_GRACE - 1))
store.insert_feedback_event(
bid,
valence=1.0,
source="user",
created_at=_fmt(fb_dt),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Retrieval counts drawn from Poisson(POISSON_LAMBDA), capped, and | |
| # scheduled at random times within each belief's lifetime (decorrelated | |
| # from age by construction: same lambda regardless of creation order). | |
| retrieval_counts = [ | |
| min(RETRIEVAL_CAP, _poisson_sample(rng, POISSON_LAMBDA)) | |
| for _ in range(N_BELIEFS) | |
| ] | |
| for i, bid in enumerate(belief_ids): | |
| creation_offset_s = creation_offsets_s[i] | |
| lifetime_s = _WINDOW_SECONDS - creation_offset_s | |
| for _ in range(retrieval_counts[i]): | |
| eligible_s = max(0, lifetime_s - T_GRACE) | |
| offset_in_lifetime = rng.randint(0, max(0, eligible_s)) | |
| enqueued_dt = _EPOCH + timedelta( | |
| seconds=creation_offset_s + offset_in_lifetime | |
| ) | |
| enqueue_retrieval_exposures(store, [bid], now=_fmt(enqueued_dt)) | |
| # Explicit feedback on a fraction of beliefs cancels their implicit rows. | |
| n_explicit = int(N_BELIEFS * EXPLICIT_FEEDBACK_FRACTION) | |
| explicit_targets = rng.sample(belief_ids, n_explicit) | |
| for bid in explicit_targets: | |
| idx = belief_ids.index(bid) | |
| creation_offset_s = creation_offsets_s[idx] | |
| fb_offset_s = creation_offset_s + (_WINDOW_SECONDS - creation_offset_s) // 2 | |
| fb_dt = _EPOCH + timedelta(seconds=fb_offset_s) | |
| store.insert_feedback_event( | |
| bid, | |
| valence=1.0, | |
| source="user", | |
| created_at=_fmt(fb_dt), | |
| ) | |
| # Retrieval counts drawn from Poisson(POISSON_LAMBDA), capped, and | |
| # scheduled at random times within each belief's lifetime (decorrelated | |
| # from age by construction: same lambda regardless of creation order). | |
| retrieval_counts = [ | |
| min(RETRIEVAL_CAP, _poisson_sample(rng, POISSON_LAMBDA)) | |
| for _ in range(N_BELIEFS) | |
| ] | |
| retrieval_times_by_belief: dict[str, list[datetime]] = {} | |
| for i, bid in enumerate(belief_ids): | |
| creation_offset_s = creation_offsets_s[i] | |
| lifetime_s = _WINDOW_SECONDS - creation_offset_s | |
| retrieval_times: list[datetime] = [] | |
| for _ in range(retrieval_counts[i]): | |
| eligible_s = max(0, lifetime_s - T_GRACE) | |
| offset_in_lifetime = rng.randint(0, max(0, eligible_s)) | |
| enqueued_dt = _EPOCH + timedelta( | |
| seconds=creation_offset_s + offset_in_lifetime | |
| ) | |
| enqueue_retrieval_exposures(store, [bid], now=_fmt(enqueued_dt)) | |
| retrieval_times.append(enqueued_dt) | |
| if retrieval_times: | |
| retrieval_times_by_belief[bid] = retrieval_times | |
| # Explicit feedback on a fraction of beliefs cancels their implicit rows. | |
| n_explicit = int(N_BELIEFS * EXPLICIT_FEEDBACK_FRACTION) | |
| explicit_targets = rng.sample( | |
| list(retrieval_times_by_belief), | |
| min(n_explicit, len(retrieval_times_by_belief)), | |
| ) | |
| for bid in explicit_targets: | |
| enqueued_dt = rng.choice(retrieval_times_by_belief[bid]) | |
| fb_dt = enqueued_dt + timedelta(seconds=rng.randint(1, T_GRACE - 1)) | |
| store.insert_feedback_event( | |
| bid, | |
| valence=1.0, | |
| source="user", | |
| created_at=_fmt(fb_dt), | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_implicit_feedback_age_correlation.py` around lines 243 - 275, The
explicit-feedback selection and timing must be tied to actual enqueued retrieval
exposures: record each enqueued_dt when calling enqueue_retrieval_exposures
(e.g., store per-bid lists like enqueued_times[bid].append(enqueued_dt]) and
choose explicit_targets from only those belief_ids with at least one enqueued
exposure (i.e., retrieval_counts[i] > 0 or enqueued_times[bid] non-empty). For
each chosen bid, pick one of its recorded enqueued_dt values and set fb_dt to a
time inside that exposure's grace window (e.g., enqueued_dt + a random offset in
[0, T_GRACE) or otherwise ensure fb_dt ∈ [enqueued_dt, enqueued_dt + T_GRACE)),
then call store.insert_feedback_event with created_at=_fmt(fb_dt) so the
explicit feedback will actually cancel a deferred implicit row (references:
retrieval_counts, enqueue_retrieval_exposures, enqueued_dt variable,
explicit_targets, store.insert_feedback_event).
Owner
Author
|
[release:review:leibniz:2026-05-10T06:02:01Z] |
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
1 participant
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 #555.
Implements the revised acceptance bullet (per operator review of #562, now closed): replace the Pearson-only guard with two non-linear correlation measures so non-monotone "becomes a clock" failure modes don't slip past.
What lands
New file
tests/test_implicit_feedback_age_correlation.py— single asserted test plus a calibration helper.Poisson(λ=2)capped at 10, retrievals scheduled at uniform random times within each belief's lifetime. Retrieval frequency is decorrelated from age by construction. ~15% of beliefs receive an explicit positive-feedback event inside the grace window (cancels the implicit row). Sweeper runs atT_end + 2·T_grace.dcor/xicordeps —dcorpulls in numba+llvmlite (~37 MB), incompatible with this repo's minimal-footprint posture. numpy is already a runtime dep.Calibrated thresholds
Calibrated 2026-05-10 via
_calibrate_thresholds()at N=200 across 30 seeds (0–29). Each threshold is the empirical 99th-percentile + 0.05 margin.Asserted-run values (seed 42): ξ = −0.0335, dCor = 0.1012 — both well below threshold, confirming the decorrelated workload produces near-zero correlation.
The calibration helper is an underscore-prefixed function (not picked up by pytest collection); rerun it from a REPL when N or the workload shape changes and update the constants.
Out of scope (unchanged from issue)
epsilon/T_grace.Verification
uv run pytest tests/test_implicit_feedback_age_correlation.py -x -q→ 1 passed.uv run pytest tests/test_implicit_feedback.py -x -q→ 29 passed (no regressions).Summary by Sourcery
Tests:
Summary by CodeRabbit