Skip to content

feat(features): implement promotion compute method (#109)#112

Merged
w7-mgfcode merged 1 commit into
devfrom
feat/featuresets-promotion
May 12, 2026
Merged

feat(features): implement promotion compute method (#109)#112
w7-mgfcode merged 1 commit into
devfrom
feat/featuresets-promotion

Conversation

@w7-mgfcode
Copy link
Copy Markdown
Owner

Summary

Lands PRP-3.1D — the promotion compute slice for time-safe featuresets. Rebased onto dev after PR #111 (lifecycle) landed.

  • Adds FeatureEngineeringService._compute_promotion_features(df, promotion_rows_df).
  • Wires it at step 7 of compute_features, gated on self.config.promotion_config.
  • Sidecar pattern via getattr(self, "_promotion_rows_df", None) — no new public API. Empty-DataFrame fallback when unset (matches the additive-contract invariant).
  • Per-kind features: promo_{kind}_active_lag{N} (Int64 nullable) + promo_{kind}_intensity_lag{N} (float64).
  • Chain-wide promotions (store_id IS NULL) handled via two-pass match — never via NaN-key merge.
  • Overlapping promos on same kind/day reduce via max(discount_pct) for intensity; active stays 0/1.
  • Sorted-kind, active-before-intensity column ordering (deterministic).

Time-safety

  • groupby(entity_cols).shift(lag_days) is the leakage gate — same-day promo MUST NOT appear in active_lag{N} at day D.
  • TestPromotionLeakage (4 cases) asserts this invariant: day-of-start = 0, day-after = 1, and chain-wide does not bleed across products.
  • test_leakage.py remains the canonical spec; no existing invariants weakened.

Scope

  • Files touched: service.py, tests/test_leakage.py, tests/test_service.py (3 files, +544/-0 vs dev).
  • No schemas.py / routes.py / conftest.py / migration / docs edits.
  • Additive-contract snapshot (config_hash_unchanged_when_phase2_omitted) still passes.
  • The end-to-end loader extension (joining promotion rows from the DB) lands in PRP-3.1E.

Rebase notes

  • Rebased from 09c9bc6037b62f on top of new dev (which contains B's lifecycle additions).
  • Conflict surface was the step-6 wiring slot in compute_features + the method-list slot just before class FeatureDataLoader: + identical class-list slots in the two test files.
  • Resolution: kept B's lifecycle additions from dev, appended D's promotion additions next to them (step-7 wiring + new method + new test classes at the end of the existing class chain). One intentional non-pickup: did not propagate D's silent deletion of assert result.feature_columns == ["lag_1"] in TestComputeFeatures::test_empty_dataframe_handling — that looked like an unintended edit artifact.

Test plan

  • ruff check + ruff format --check clean
  • pytest -v on app/features/featuresets/ — 95 tests pass (4 new leakage + 9 new service cases on top of the existing slice)
  • CI green on PR

Refs

Part of #109 (Phase 2 feature wiring). PRP-3.1E will land the loader extension that populates _promotion_rows_df from the DB join.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 12, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 69424eda-9a55-41ce-83aa-6f07a1c407df

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/featuresets-promotion

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @w7-mgfcode, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant