Skip to content

feat: make label_window_days affect conversion label derivation#43

Merged
shaypal5 merged 3 commits intomainfrom
feat/label-window-affects-simulation
May 1, 2026
Merged

feat: make label_window_days affect conversion label derivation#43
shaypal5 merged 3 commits intomainfrom
feat/label-window-affects-simulation

Conversation

@shaypal5
Copy link
Copy Markdown
Contributor

@shaypal5 shaypal5 commented May 1, 2026

Closes #41

Summary

  • simulation/engine.py: converted_within_90_days is now gated by config.label_window_days — a lead is only labeled positive if its conversion event occurred before day label_window_days (not the full horizon_days). The simulation still runs for the full horizon to produce rich event histories; only label derivation is affected.
  • schema/entities.py: LeadRow docstring updated to document that the field uses label_window_days (field name retained for schema stability).
  • 6 new tests covering: default behavior unchanged, shorter window → fewer conversions, 1-day window → zero conversions, late conversions excluded from label, conversion_timestamp still set for actual conversions outside window, event counts unchanged by window.

Design decisions

  • No field rename: converted_within_90_days is kept as-is per issue guidance — renaming would touch too many files across schema, rendering, validation, and pipelines.
  • Label-only change: label_window_days only affects the boolean label, not conversion_timestamp, event generation, or post-simulation entity creation (opportunities, customers, subscriptions).
  • Boundary: conversion_day < label_window_days (exclusive upper bound), matching the existing range(horizon_days) convention where day indices are 0-based.

Test plan

  • All 787 existing tests pass (no regressions)
  • test_default_90_unchanged — labels identical to old behavior with default config
  • test_shorter_window_fewer_conversions — 30-day window strictly fewer than 90-day
  • test_window_1_almost_no_conversions — 1-day window produces 0 conversions
  • test_late_conversions_excluded — positive labels only for early conversions
  • test_conversion_timestamp_still_set_outside_window — late converters have timestamp but label=False
  • test_event_count_unchanged_by_window — touches/sessions/activities identical regardless of window

🤖 Generated with Claude Code

@shaypal5 @claude
feat: make label_window_days affect conversion label derivation (#41)
70eb2aa 
The simulation engine now gates `converted_within_90_days` by
`config.label_window_days` instead of using the full horizon.
Simulation still runs for `horizon_days` to produce rich event
histories; only label derivation respects the label window.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings May 1, 2026 20:45
@shaypal5 shaypal5 added type: feature New capability layer: core core/ primitives (RNG, IDs, models, exceptions) layer: simulation simulation/ discrete-time engine labels May 1, 2026
@github-actions

This comment has been minimized.

Sign in to view
@shaypal5 @claude
fix: strengthen tests and document feature-label window mismatch
ad51b6d 
- test_late_conversions_excluded now verifies conversion day offset < 30
  (was a tautology — only checked timestamp not None)
- Add bundle round-trip integration test (Generator → save → read Parquet)
- Document feature-label temporal mismatch in build_snapshot when
  label_window_days < horizon_days

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@github-actions

This comment has been minimized.

Sign in to view
Copilot AI reviewed May 1, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Updates the simulation engine so the binary conversion label (converted_within_90_days) is derived using GenerationConfig.label_window_days rather than always using the full simulation horizon, while keeping the simulation itself running for horizon_days.

Changes:

  • Gate converted_within_90_days on conversion_day < config.label_window_days in simulate_world().
  • Update LeadRow documentation to clarify the label is driven by label_window_days (name retained for schema stability).
  • Add unit tests covering several label_window_days scenarios and ensuring event generation is unchanged.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.

File Description
tests/simulation/test_engine.py Extends test config helper to accept label_window_days and adds a new test class validating label-window behavior.
leadforge/simulation/engine.py Computes converted_within_90_days from conversion timing relative to label_window_days, not the full horizon.
leadforge/schema/entities.py Expands LeadRow docstring explaining converted_within_90_days now reflects label_window_days.
.agent-plan.md Updates the agent plan log to reflect the completed work/tests for the label-window change.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/simulation/test_engine.py
Comment thread tests/simulation/test_engine.py
Comment thread leadforge/schema/entities.py
@shaypal5 @claude
docs: note that conversion_timestamp can be set when label is False
8c39c07 
Address Copilot review: document the intentional case where a lead
converts after the label window, producing a non-None
conversion_timestamp with converted_within_90_days=False.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 1, 2026

pr-agent-context report:

No unresolved review comments, failing checks, or actionable patch coverage gaps were found on PR #43 in repository https://github.com/leadforge-dev/leadforge. Treat this PR as all clear unless new signals appear.

Run metadata:

Tool ref: v4
Tool version: 4.0.21
Trigger: commit pushed
Workflow run: 25232756350 attempt 1
Comment timestamp: 2026-05-01T20:55:26.914368+00:00
PR head commit: 8c39c07b6197e5e7c9a1e89b18711baa1d9757f3

@shaypal5 shaypal5 merged commit c093faf into main May 1, 2026
7 checks passed
@shaypal5 shaypal5 deleted the feat/label-window-affects-simulation branch May 1, 2026 20:57
@shaypal5 shaypal5 mentioned this pull request May 3, 2026
Merged
3 tasks
shaypal5 added a commit that referenced this pull request May 3, 2026
@shaypal5 @claude
fix: tune v6 for post-engine-change validation — snapshot day 20, Poi…
c01080d 
…sson(3) boost

Engine changes in PRs #40, #43, #45 weakened the v6 dataset signal.
Two tuning changes restore all validation metrics:

1. Shift SNAPSHOT_DAY from 14 to 20 — features need more accumulation
   time after engine changes; day-14 AUC was 0.611, day-20 is 0.676.

2. Poisson(3) trap boost for converted leads — the wider snapshot
   window leaves fewer post-snapshot days for causal trap signal,
   so a stronger boost compensates (mean delta 0.061, min 0.027).

All mandatory checks pass with comfortable margins. AUC threshold
kept at 0.62 (no relaxation needed).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
shaypal5 added a commit that referenced this pull request May 3, 2026
@shaypal5 @claude
fix: tune v6 pipeline for post-engine-change validation (#49)
0ac1090 
* fix: tune v6 pipeline for post-engine-change validation

Engine changes in PRs #40, #43, #45 weakened the v6 dataset signal.
Two fixes:

1. Add Poisson(1) boost to the leakage trap column for converted leads
   (same approach proven in v5), restoring robust trap delta signal
   (mean 0.048, min 0.028 across 10 seeds).

2. Lower baseline AUC threshold from 0.62 to 0.60 — the engine changes
   reduced baseline LR AUC from 0.667 to 0.611, which is still well
   above chance and pedagogically useful. Snapshot day 10 was tested
   but made AUC worse (0.572), so day 14 is retained.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* docs: update .agent-plan.md with v6 retune validation results

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: tune v6 for post-engine-change validation — snapshot day 20, Poisson(3) boost

Engine changes in PRs #40, #43, #45 weakened the v6 dataset signal.
Two tuning changes restore all validation metrics:

1. Shift SNAPSHOT_DAY from 14 to 20 — features need more accumulation
   time after engine changes; day-14 AUC was 0.611, day-20 is 0.676.

2. Poisson(3) trap boost for converted leads — the wider snapshot
   window leaves fewer post-snapshot days for causal trap signal,
   so a stronger boost compensates (mean delta 0.061, min 0.027).

All mandatory checks pass with comfortable margins. AUC threshold
kept at 0.62 (no relaxation needed).

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* docs: update RELEASE_v6.md metrics for retuned pipeline

Snapshot day 14→20, updated all baseline AUC, trap delta, value-aware
ranking, and teaching guidance numbers to match retuned pipeline.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

* fix: address Copilot review — add boost tests, fix stale docstrings

COPILOT-1: Add 5 unit tests for boost_leakage_trap() covering:
only converted leads boosted, input immutability, determinism,
converted leads >= original, mean increases after boost.

COPILOT-2: Update build script docstring and inline comment to
reflect that the trap is no longer purely causal — it combines
causal post-snapshot touches with a Poisson(3) boost.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

Assignees

No one assigned

Labels

layer: core core/ primitives (RNG, IDs, models, exceptions) layer: simulation simulation/ discrete-time engine type: feature New capability

Projects

None yet

Milestone

v0.3.0 — Motif variability + exposure...

Development

Successfully merging this pull request may close these issues.

Make label_window_days affect simulation and snapshot behavior

2 participants

@shaypal5