feat(features): lower ReplenishmentConfig count_window_days floor to 3 (#113)#122
Conversation
#113) PRP-3.1C's pseudocode originally referenced W=3 as the smallest meaningful trailing window for replenishment-count features, but PRP-3.1A's schema set `ge=7` with no recorded rationale. The mismatch forced `test_count_window_uses_shift_then_rolling` to use W=7 instead of the original W=3 probe. This change aligns the schema with PRP-3.1C's intent: - `count_window_days = Field(default=14, ge=3, le=60)` (was `ge=7`) - Update boundary test to probe the new floor (count_window_days=2) - Refresh leakage-test docstring and docs/PHASE/3 range comment All existing call sites already use W=7 or W=14, so no behavioral regression. `shift(1).rolling(W, min_periods=1).sum()` works for any W>=1; W=3 is the smallest window that can detect a cadence trend. Closes #113.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 guide (collapsed on small PRs)Reviewer's GuideLowers the minimum allowed ReplenishmentConfig.count_window_days from 7 to 3 and updates validation tests, leakage-test documentation, and feature-engineering docs to match the expanded configuration range. File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The min/max bounds for
count_window_days(3 and 60) are currently duplicated across the schema docstring, tests, and docs; consider centralizing these values (e.g., as module-level constants or config) to reduce the chance of future range changes getting out of sync.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The min/max bounds for `count_window_days` (3 and 60) are currently duplicated across the schema docstring, tests, and docs; consider centralizing these values (e.g., as module-level constants or config) to reduce the chance of future range changes getting out of sync.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Summary
Closes #113. Aligns
ReplenishmentConfig.count_window_daysfloor with PRP-3.1C's original pseudocode intent (W=3) — PRP-3.1A'sge=7was set without recorded rationale, forcing the leakage testtest_count_window_uses_shift_then_rollingto use W=7 instead of the W=3 probe the PRP described.Diff
schemas.py—count_window_days = Field(default=14, ge=3, le=60)(wasge=7); docstring range comment(7-60)→(3-60).test_schemas.py— boundary test now probescount_window_days=2to verify the new floor (previously=6).test_leakage.py— docstring updated to drop the stale "smallest window the config schema allows" claim; test body unchanged (still uses W=7, which now sits comfortably above the floor).docs/PHASE/3-FEATURE_ENGINEERING.md— inline range comment# 7..60→# 3..60.Test plan
uv run ruff check app/features/featuresets/— cleanuv run ruff format --check app/features/featuresets/— 11 files already formatteduv run mypy app/— 0 issues in 192 filesuv run pyright app/features/featuresets/— 0 errorsuv run pytest app/features/featuresets/ -m "not integration"— 108 passeduv run pytest -m "not integration"— 962 passed (full unit sweep)Notes for the reviewer
ge=3. The change is purely an input-range expansion.shift(1).rolling(W, min_periods=1).sum()works for anyW >= 1. W=3 is the smallest window that can detect a cadence trend (W=1 is justshift(1), W=2 is the minimum smoothing). Sub-weekly cadence windows are meaningful for retailers with daily replenishment.shift(1).rolling(W).count()ordering, not the floor. W=7 still demonstrates the math; only the docstring's parenthetical needed updating.Out of scope
_compute_replenishment_featuresmath (per the issue's "Out of scope").test_leakage.py::TestReplenishmentLeakageinvariants (untouched).Summary by Sourcery
Lower the minimum allowed replenishment count window to broaden configuration options while keeping existing behavior intact.
New Features:
Enhancements: