Card Evolution: season stats full recalculation → next-release #112

cal · 2026-03-19T15:39:53Z

cal commented

2026-03-19 15:39:53 +00:00

Summary

Promotes the season stats full-recalculation refactor (PR #111) from card-evolution into next-release.

Changes

update_season_stats() rewritten from incremental delta upserts to full recalculation from StratPlay rows — eliminates double-counting and enables self-healing via force=True
evolution_evaluator.py fixed — broken PlayerSeasonStats import replaced with correct BattingSeasonStats/PitchingSeasonStats queries based on card_type; r.k → r.strikeouts
Router updated — force query param passthrough on POST /api/v2/season-stats/update-game/{game_id}
Test fixes — PlayerSeasonStats → BattingSeasonStats in test_evolution_models.py and test_postgame_evolution.py
4 new tests — force_recalc, idempotent_reprocessing, partial_reprocessing_heals, decision_only_pitcher

Testing

Full test suite: 116 passed, 15 skipped (PG integration), 0 failed. Three rounds of PR review completed.

🤖 Generated with Claude Code

## Summary Promotes the season stats full-recalculation refactor (PR #111) from `card-evolution` into `next-release`. ## Changes - **`update_season_stats()` rewritten** from incremental delta upserts to full recalculation from StratPlay rows — eliminates double-counting and enables self-healing via `force=True` - **`evolution_evaluator.py` fixed** — broken `PlayerSeasonStats` import replaced with correct `BattingSeasonStats`/`PitchingSeasonStats` queries based on card_type; `r.k` → `r.strikeouts` - **Router updated** — `force` query param passthrough on `POST /api/v2/season-stats/update-game/{game_id}` - **Test fixes** — `PlayerSeasonStats` → `BattingSeasonStats` in `test_evolution_models.py` and `test_postgame_evolution.py` - **4 new tests** — force_recalc, idempotent_reprocessing, partial_reprocessing_heals, decision_only_pitcher ## Testing Full test suite: 116 passed, 15 skipped (PG integration), 0 failed. Three rounds of PR review completed. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

cal added 5 commits 2026-03-19 15:39:54 +00:00

refactor: replace incremental delta upserts with full recalculation in season stats 1b4eab9d99

The previous approach accumulated per-game deltas into season stats rows,
which was fragile — partial processing corrupted stats, upsert bugs
compounded, and there was no self-healing mechanism.

Now update_season_stats() recomputes full season totals from all StratPlay
rows for each affected player whenever a game is processed. The result
replaces whatever was stored, eliminating double-counting and enabling
self-healing via force=True.

Also fixes:
- evolution_evaluator.py: broken PlayerSeasonStats import → queries
  BattingSeasonStats or PitchingSeasonStats based on card_type
- evolution_evaluator.py: r.k → r.strikeouts
- test_evolution_models.py, test_postgame_evolution.py: PlayerSeasonStats
  → BattingSeasonStats (model never existed)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

fix: address PR review — correct Peewee DISTINCT syntax and Decision-only pitchers 4211bd69e0

- fn.COUNT(fn.DISTINCT(expr)) → fn.COUNT(expr.distinct()) for correct
  COUNT(DISTINCT ...) SQL on PostgreSQL
- _get_player_pairs() now also scans Decision table to include pitchers
  who have a Decision row but no StratPlay rows (rare edge case)
- Updated stale docstring references to PlayerSeasonStats and r.k

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

fix: stale docstring + add decision-only pitcher test 46c85e6874

- evaluate_card() docstring: "Override for PlayerSeasonStats" →
  "Override for BattingSeasonStats/PitchingSeasonStats"
- New test_decision_only_pitcher: exercises the edge case where a pitcher
  has a Decision row but no StratPlay rows, verifying _get_player_pairs()
  correctly includes them via the Decision table scan

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

docs: update stale docstrings to reflect full-recalculation approach d10276525e

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Merge pull request 'refactor: full recalculation for season stats + fix evaluator bugs' (#111 ) from feature/season-stats-full-recalc into card-evolution

Build Docker Image / build (pull_request) Successful in 8m11s

Details

b68e280c64

Reviewed-on: #111

Claude added the

ai-reviewing

label 2026-03-19 15:45:30 +00:00

Claude approved these changes 2026-03-19 15:47:48 +00:00

Claude left a comment

AI Code Review

Files Reviewed

app/routers_v2/season_stats.py (modified)
app/services/season_stats.py (modified — full rewrite)
app/services/evolution_evaluator.py (modified)
tests/test_evolution_evaluator.py (modified)
tests/test_evolution_models.py (modified)
tests/test_postgame_evolution.py (modified)
tests/test_season_stats_update.py (modified — 4 new tests)

Findings

Correctness

Full recalculation logic is correct. The new _recalc_batting and _recalc_pitching helpers aggregate from StratPlay via a JOIN to StratGame and filter by season. This is semantically correct — the full-season totals are computed from the source of truth rather than accumulated via deltas.

games count for batters (_recalc_batting). Uses fn.COUNT(Case(None, [(StratPlay.pa > 0, StratPlay.game)], None).distinct()). The Case expression returns game_id when pa > 0 and NULL otherwise; COUNT ignores NULL; .distinct() ensures a player appearing multiple times in a game with pa > 0 is counted once. This is correct and matches the prior logic (g["games"] = 1 on first pa > 0 play).

games count for pitchers (_recalc_pitching). Uses fn.COUNT(StratPlay.game.distinct()) — counts all distinct game IDs regardless of play type. This matches the prior behavior where "games": 1 was set unconditionally for all pitchers.

_recalc_decisions uses Decision.season directly. The Decision model has a season = IntegerField() column (not a FK), so the WHERE Decision.season == season filter is correct. This is the right table to query rather than joining through StratGame.

evolution_evaluator.py fix is correct. The r.k attribute error (old PlayerSeasonStats model had k, but BattingSeasonStats/PitchingSeasonStats use strikeouts) is resolved. The new branching logic on card_type properly routes to BattingSeasonStats or PitchingSeasonStats and queries with player == / team == FK fields — Peewee resolves integer comparisons against FK field names correctly.

force=True path is correct. When not created and force=True, the code logs and continues — it doesn't delete or update the ProcessedGame ledger row, which is the right behavior (the row still exists to track that the game was first processed).

_stats_model override path in evolution_evaluator.py is correct. The test injection path (if _stats_model is not None) uses _stats_model.player_id and _stats_model.team_id — these are raw FK column accessors consistent with how the test stub model is defined (using IntegerField directly). The production path (when _stats_model is None) uses BattingSeasonStats.player / .team FK fields — also correct.

get_or_create in update_season_stats uses raw FK accessors (player_id=, team_id=). This is the established codebase pattern (confirmed in prior PR reviews #106, #107, #108) and is valid.

obj.last_game_id = game_id — The model defines last_game = ForeignKeyField(StratGame, null=True). Setting last_game_id (the raw column) directly to an integer is valid in Peewee and avoids an unnecessary FK object fetch. Consistent with prior usage in the sqlite upsert paths being replaced.

New docstring on _CareerTotals correctly updated from sp/rp: outs, k to sp/rp: outs, strikeouts.

Decision-only pitcher test is correct. test_decision_only_pitcher asserts stats.games == 0 because the relief pitcher has no StratPlay rows, so COUNT(DISTINCT game) from _recalc_pitching returns 0. This is an intentional, documented behavior change from the old code (which set "games": 1 unconditionally in _apply_decisions). The comment in _get_player_pairs acknowledges this edge case.

Security

No regressions. The [REDACTED] log pattern is preserved in season_stats.py (router). No user-controlled data flows into raw SQL. The force query param is a boolean — no injection surface.

Style & Conventions

The PR follows established codebase patterns throughout: lazy imports at call time (from app.db_engine import ... inside the else block), get_or_create with raw FK accessors, db.atomic() wrapping, logger.info/debug with %-style formatting.

One minor observation: The _recalc_batting docstring still says "The COUNT(DISTINCT ...) with a CASE expression achieves this: NULL values from the CASE are ignored by COUNT" but the actual query uses Peewee's .distinct() chained onto the Case(...) expression, which Peewee renders as COUNT(DISTINCT CASE ...). This is technically the same thing but the description could be clearer. Non-blocking.

Suggestions

test_decision_only_pitcher asserts stats.games == 0: This is a documented intentional behavior, but it means a pitcher who received a win in a game they don't appear in via StratPlay rows will show games=0, wins=1 in their season stats — which is a valid edge case rather than a bug. Worth a comment in the test explaining this is intentional.
No multi-season career accumulation test for evolution_evaluator.py: The new else branch sums over all seasons. The existing test coverage in test_postgame_evolution.py seeds a prior-season row (season=10) and a current game (season=11), which indirectly exercises this. Adequate coverage.
Concurrent force=True calls: If two requests race with force=True for the same game_id, both bypass the not created guard and write simultaneously. Since get_or_create + field assignment + save() is not atomic at the SQL level, a race could produce a partial write. This is the same risk documented in PR #106 and the full-recalculation approach actually makes it less dangerous (worst case: one overwrite wins, stats are still correct totals), so this is acceptable.

Verdict: APPROVED

This PR correctly promotes the full-recalculation refactor from card-evolution to next-release. The core rewrite eliminates the double-counting and out-of-order replay problems inherent to the incremental delta approach. All three helper functions (_recalc_batting, _recalc_pitching, _recalc_decisions) produce correct SQL aggregations. The evolution_evaluator.py fix resolves the broken PlayerSeasonStats import and r.k attribute error. The force=True passthrough is clean. Test coverage is thorough (4 new tests covering force recalc, idempotency, self-healing, and decision-only pitcher).

Automated review by Claude PR Reviewer

## AI Code Review ### Files Reviewed - `app/routers_v2/season_stats.py` (modified) - `app/services/season_stats.py` (modified — full rewrite) - `app/services/evolution_evaluator.py` (modified) - `tests/test_evolution_evaluator.py` (modified) - `tests/test_evolution_models.py` (modified) - `tests/test_postgame_evolution.py` (modified) - `tests/test_season_stats_update.py` (modified — 4 new tests) ### Findings #### Correctness **Full recalculation logic is correct.** The new `_recalc_batting` and `_recalc_pitching` helpers aggregate from `StratPlay` via a `JOIN` to `StratGame` and filter by `season`. This is semantically correct — the `full-season` totals are computed from the source of truth rather than accumulated via deltas. **`games` count for batters (`_recalc_batting`).** Uses `fn.COUNT(Case(None, [(StratPlay.pa > 0, StratPlay.game)], None).distinct())`. The `Case` expression returns `game_id` when `pa > 0` and `NULL` otherwise; `COUNT` ignores `NULL`; `.distinct()` ensures a player appearing multiple times in a game with `pa > 0` is counted once. This is correct and matches the prior logic (`g["games"] = 1` on first `pa > 0` play). **`games` count for pitchers (`_recalc_pitching`).** Uses `fn.COUNT(StratPlay.game.distinct())` — counts all distinct game IDs regardless of play type. This matches the prior behavior where `"games": 1` was set unconditionally for all pitchers. **`_recalc_decisions` uses `Decision.season` directly.** The Decision model has a `season = IntegerField()` column (not a FK), so the `WHERE Decision.season == season` filter is correct. This is the right table to query rather than joining through StratGame. **`evolution_evaluator.py` fix is correct.** The `r.k` attribute error (old `PlayerSeasonStats` model had `k`, but `BattingSeasonStats`/`PitchingSeasonStats` use `strikeouts`) is resolved. The new branching logic on `card_type` properly routes to `BattingSeasonStats` or `PitchingSeasonStats` and queries with `player ==` / `team ==` FK fields — Peewee resolves integer comparisons against FK field names correctly. **`force=True` path is correct.** When `not created and force=True`, the code logs and continues — it doesn't delete or update the ProcessedGame ledger row, which is the right behavior (the row still exists to track that the game was first processed). **`_stats_model` override path in `evolution_evaluator.py` is correct.** The test injection path (`if _stats_model is not None`) uses `_stats_model.player_id` and `_stats_model.team_id` — these are raw FK column accessors consistent with how the test stub model is defined (using `IntegerField` directly). The production path (when `_stats_model is None`) uses `BattingSeasonStats.player` / `.team` FK fields — also correct. **`get_or_create` in `update_season_stats` uses raw FK accessors (`player_id=`, `team_id=`).** This is the established codebase pattern (confirmed in prior PR reviews #106, #107, #108) and is valid. **`obj.last_game_id = game_id`** — The model defines `last_game = ForeignKeyField(StratGame, null=True)`. Setting `last_game_id` (the raw column) directly to an integer is valid in Peewee and avoids an unnecessary FK object fetch. Consistent with prior usage in the sqlite upsert paths being replaced. **New docstring on `_CareerTotals` correctly updated** from `sp/rp: outs, k` to `sp/rp: outs, strikeouts`. **Decision-only pitcher test is correct.** `test_decision_only_pitcher` asserts `stats.games == 0` because the relief pitcher has no StratPlay rows, so `COUNT(DISTINCT game)` from `_recalc_pitching` returns 0. This is an intentional, documented behavior change from the old code (which set `"games": 1` unconditionally in `_apply_decisions`). The comment in `_get_player_pairs` acknowledges this edge case. #### Security No regressions. The `[REDACTED]` log pattern is preserved in `season_stats.py` (router). No user-controlled data flows into raw SQL. The `force` query param is a boolean — no injection surface. #### Style & Conventions The PR follows established codebase patterns throughout: lazy imports at call time (`from app.db_engine import ...` inside the `else` block), `get_or_create` with raw FK accessors, `db.atomic()` wrapping, `logger.info/debug` with `%`-style formatting. One minor observation: The `_recalc_batting` docstring still says "The `COUNT(DISTINCT ...)` with a `CASE` expression achieves this: NULL values from the CASE are ignored by COUNT" but the actual query uses Peewee's `.distinct()` chained onto the `Case(...)` expression, which Peewee renders as `COUNT(DISTINCT CASE ...)`. This is technically the same thing but the description could be clearer. Non-blocking. #### Suggestions - **`test_decision_only_pitcher` asserts `stats.games == 0`**: This is a documented intentional behavior, but it means a pitcher who received a win in a game they don't appear in via StratPlay rows will show `games=0, wins=1` in their season stats — which is a valid edge case rather than a bug. Worth a comment in the test explaining this is intentional. - **No multi-season career accumulation test for `evolution_evaluator.py`**: The new `else` branch sums over all seasons. The existing test coverage in `test_postgame_evolution.py` seeds a prior-season row (`season=10`) and a current game (`season=11`), which indirectly exercises this. Adequate coverage. - **Concurrent force=True calls**: If two requests race with `force=True` for the same `game_id`, both bypass the `not created` guard and write simultaneously. Since `get_or_create` + field assignment + `save()` is not atomic at the SQL level, a race could produce a partial write. This is the same risk documented in PR #106 and the full-recalculation approach actually makes it *less* dangerous (worst case: one overwrite wins, stats are still correct totals), so this is acceptable. ### Verdict: APPROVED This PR correctly promotes the full-recalculation refactor from `card-evolution` to `next-release`. The core rewrite eliminates the double-counting and out-of-order replay problems inherent to the incremental delta approach. All three helper functions (`_recalc_batting`, `_recalc_pitching`, `_recalc_decisions`) produce correct SQL aggregations. The `evolution_evaluator.py` fix resolves the broken `PlayerSeasonStats` import and `r.k` attribute error. The `force=True` passthrough is clean. Test coverage is thorough (4 new tests covering force recalc, idempotency, self-healing, and decision-only pitcher). --- *Automated review by Claude PR Reviewer*

cal merged commit cf0b1d1d1c into next-release

2026-03-19 15:49:11 +00:00

cal referenced this issue from a commit

2026-03-19 15:49:13 +00:00

Merge pull request 'Card Evolution: season stats full recalculation → next-release' (#112) from card-evolution into next-release

cal removed the

ai-reviewing

label 2026-03-23 15:32:59 +00:00

Sign in to join this conversation.

No reviewers

No Milestone

No project

No Assignees

2 Participants

Notifications

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: cal/paper-dynasty-database#112