gabogg/hikcentral
Watch
1
0
Fork
You've already forked hikcentral
 0
Code Issues 113 Pull requests 8 Projects Releases Packages Wiki Activity Actions

feat(statistics): period KPIs and entrance comparisons (#83, #86) #92

Merged
gabogg merged 10 commits from feat/statistics-summary-entrances into master 2026-09-25 12:17:29 +00:00
Conversation 2 Commits 10 Files changed 7 +815 -12
gabogg commented 2026-09-25 01:15:54 +00:00
Owner
Copy link

Summary

Implements #83 and #86: a selected-period KPI strip with previous and last-year comparisons, and counted-camera entrance shares with explicit new-camera and estimated-attribution states. This PR is stacked on #90, which is stacked on #89.

Architectural impact

Adds two authenticated, read-only /api/statistics/ routes and typed response contracts. Aggregation remains in AnalyticsService; counted per-camera SQL remains in OccupancyRepository. No schema migration or write path.

Verification

  • pytest -q: full suite passed after review fixes.
  • Pre-commit: Ruff lint, Ruff format, pytest passed.
  • Documentation check: 33 Markdown files and 72 HTTP operations.

Checklist

  • #83: day/week/month KPI summaries, partial-period denominator, open-window dwell, source-aware comparisons and quality. Historical peaks use each cycle's trusted multiplier; period dwell is weighted by open-window visitors.
  • #86: visitor and egress totals, share, previous/last-year comparison, new-camera and multi-camera estimate markers.
  • Review stacked dependencies #89 and #90 before retargeting to master.

Depends on #90 and #89. Refs #80, #83, #86.

Review follow-up

A partial current or reference week/month returns reason: PARTIAL_PERIOD with no percentage change. Last-year alignment and comparison-null semantics are documented in the API reference.

## Summary Implements #83 and #86: a selected-period KPI strip with previous and last-year comparisons, and counted-camera entrance shares with explicit new-camera and estimated-attribution states. This PR is stacked on #90, which is stacked on #89. ## Architectural impact Adds two authenticated, read-only `/api/statistics/` routes and typed response contracts. Aggregation remains in `AnalyticsService`; counted per-camera SQL remains in `OccupancyRepository`. No schema migration or write path. ## Verification - `pytest -q`: full suite passed after review fixes. - Pre-commit: Ruff lint, Ruff format, pytest passed. - Documentation check: 33 Markdown files and 72 HTTP operations. ## Checklist - [x] #83: day/week/month KPI summaries, partial-period denominator, open-window dwell, source-aware comparisons and quality. Historical peaks use each cycle's trusted multiplier; period dwell is weighted by open-window visitors. - [x] #86: visitor and egress totals, share, previous/last-year comparison, new-camera and multi-camera estimate markers. - [ ] Review stacked dependencies #89 and #90 before retargeting to `master`. Depends on #90 and #89. Refs #80, #83, #86. ## Review follow-up A partial current or reference week/month returns `reason: PARTIAL_PERIOD` with no percentage change. Last-year alignment and comparison-null semantics are documented in the API reference.
gabogg changed title from WIP: feat(statistics): period KPIs and entrance comparisons (#83, #86) to feat(statistics): period KPIs and entrance comparisons (#83, #86) 2026-09-25 08:31:53 +00:00
gabogg commented 2026-09-25 08:39:18 +00:00
Author
Owner
Copy link

Code review — pass 1 (two-axis)

Reviewed the PR's own diff (git diff <base>...<head>); Standards and Spec ran as independent passes and are reported separately, not reranked. Severity: P1 must fix · P2 fix before merge · P3 minor. Per review policy, pass 1 findings are all to be fixed on the branch.

Standards

The diff breaks no documented standard. Layering holds: SQL stays in OccupancyRepository, controllers stay thin, and errors use X-Error-Code per code-standards.md §3. Both new routes have tests.

Bugs / correctness

  • P2, analytics_service.py:210-219, 175-183. The current visitors total counts only covered days, but change_percent compares it with the full reference period. A month with missing cycles shows a false drop. The partial-period denominator is used only for daily_average. Scale the comparison to covered days or suppress it when quality.covered_days is less than the period length.
  • P3, analytics_service.py:346-351. is_new checks only first_event_epoch >= current_epoch. A camera whose first event falls after the period is shown as NEW with 0 visitors. Also check the first event is before the period end.
  • P3, analytics_service.py:481-491 vs get_dwell_dayparts_async (~589). Daily-series dwell and peak now use each cycle's trusted multiplier; /statistics/dwell/dayparts still uses active_exit_multiplier, so the same day can show two dwell values.

Documented-standard notes

  • P3, PR metadata. The description calls this a draft, but the title has no WIP: prefix (git-and-workflow.md §2.1, AGENTS.md §4).
  • P3, schemas/statistics.py:176. New contract field EntranceStatistics.zone_name; CONTEXT.md lists "Zone" under Avoid for Camera Group. Follows the existing column, so low priority.

Baseline smells (judgement calls)

  • P3, possible Duplicated Code:
    • The comparison block change_percent=round(100 * (x / ref - 1), 1) if ref else None, reason="ZERO_REFERENCE_VISITORS" if ref == 0 else None appears four times (analytics_service.py:181-182, 268-269, 342-343, 364-366). Extract a single VisitorComparison factory.
    • Trusted-multiplier selection at :481-491 repeats the logic at :773-780.
    • Each route repeats the same ValueError→422 / LookupError→404 try/except (statistics_controller.py:40-47, 58-65).
  • P3, possible Primitive Obsession, schemas/statistics.py:106. VisitorComparison.reason: str holds ~seven fixed codes (NEW_CAMERA, NO_DATA_LAST_YEAR, …), while attribution and status in the same file use Literal. Make reason a Literal.
  • P3, possible Repeated Switches, analytics_service.py:225-228, 250-300. get_statistics_summary_async (~110 lines) checks granularity in two != "day" conditions then again in an if/elif/else. One helper per granularity would shorten it.
  • P3, possible Mysterious Name, schemas/statistics.py:148-149. busiest_day (week) and best_day (month) hold the same value from the same variable. Use one name.
  • P3, possible Mysterious Name, analytics_service.py:334. old, prior, top are unclear; previous_row / previous_comparison would read better.

Spec

Nothing blocks merge. Both routes, every field the mockup lists, require_auth, the closed-period check, counted-camera filtering and the triage decisions are in place.

(a) Missing or partial

  • P2 · No tests for several claimed behaviours (tests/test_statistics_summary.py). No test for:

    • a week summary (busiest_day, week weekend share);
    • any case where last-year data exists, for the summary or the entrances route;
    • ISO-week or Feb-29 last-year alignment;
    • the NO_DATA_PREVIOUS_PERIOD reason;
    • the 404 when a period has no data;
    • the NO_REFERENCE_DATA status;
    • dwell weighted by open-window visitors.

    The spec requires "vs same period last year (only when last year has data…)", but only the omitted branch is tested.

  • P3 · Spec: "Each comparison returns null with a reason". The code returns a VisitorComparison object with reference_visitors: null plus reason, not a literal null. Fine if the deck expects the object; document it.

(b) Scope creep

  • P3 · analytics_service.py:476-490: /timeseries/daily peaks now use each cycle's trusted multiplier. Changes #84's route (not asked for here, though sound — and #89's spec review asks for exactly this). get_dwell_dayparts_async (:589) still uses the active multiplier, so dwell for the same day can differ between the summary and the dayparts route.
  • P3 · :350: adds a third status, NO_REFERENCE_DATA, beyond the NEW rule in the triage. Reasonable, but new contract surface.

(c) Implemented but questionable

  • P2 · Partial-period comparisons (:170, :216). Triage: "Partial periods: figures cover only days with data". The current total covers only its covered days, but the reference sum is the raw total of the reference period. A 2-of-30-day month compared with a full month shows roughly −90%. Normalise by covered days or return the reference period's covered_days.
  • P3 · Cameras with no current flow (occupancy_repository.py:1259, :331). Triage: "a camera that disappeared is not listed". An active camera with zero events this period is still listed as EXISTING at −100%. If "disappeared" means "no events", this contradicts the rule. Clarify.
  • P3 · Last-year detection on entrances (:326). has_year_data looks only at current cameras. If last year's data came only from cameras since removed, last_year is omitted even though the summary route shows it.
  • P3 · tests/test_statistics_summary.py:139: the unaligned-week check is skipped whenever the test runs on a Monday.

Open design points

Point Verdict
New or removed cameras (#86) Decided and sound: first event ≥ period start means NEW; inactive cameras dropped
Multi-camera estimates (#86) Decided and sound: attribution: ESTIMATED
Last-year alignment Decided and sound: ISO week, calendar month, calendar day with Feb 29 → Feb 28 (:94). Undocumented
Partial periods versus comparisons Decided but questionable (see (c))
Weekend share (Saturday and Sunday business days) Decided and sound
Usual weekday fewer than 2 samples Decided and sound: INSUFFICIENT_USUAL_WEEKDAYS with source_dates
Mean dwell over the period Decided and sound: weighting by open-window visitors equals pooled Little's Law

Summary: Standards 12 (1×P2, 11×P3) — worst: comparisons pit covered-days-only totals against full reference periods. Spec 8 (2×P2, 6×P3) — worst: same partial-period comparison flaw, plus missing tests for last-year-present and week paths.

🤖 Generated with Claude Code

# Code review — pass 1 (two-axis) Reviewed the PR's own diff (`git diff <base>...<head>`); Standards and Spec ran as independent passes and are reported separately, not reranked. Severity: **P1** must fix · **P2** fix before merge · **P3** minor. Per review policy, pass 1 findings are all to be fixed on the branch. ## Standards The diff breaks no documented standard. Layering holds: SQL stays in `OccupancyRepository`, controllers stay thin, and errors use `X-Error-Code` per `code-standards.md` §3. Both new routes have tests. ### Bugs / correctness - **P2, `analytics_service.py:210-219, 175-183`.** The current visitors total counts only covered days, but `change_percent` compares it with the full reference period. A month with missing cycles shows a false drop. The partial-period denominator is used only for `daily_average`. Scale the comparison to covered days or suppress it when `quality.covered_days` is less than the period length. - **P3, `analytics_service.py:346-351`.** `is_new` checks only `first_event_epoch >= current_epoch`. A camera whose first event falls *after* the period is shown as `NEW` with 0 visitors. Also check the first event is before the period end. - **P3, `analytics_service.py:481-491` vs `get_dwell_dayparts_async` (~589).** Daily-series dwell and peak now use each cycle's trusted multiplier; `/statistics/dwell/dayparts` still uses `active_exit_multiplier`, so the same day can show two dwell values. ### Documented-standard notes - **P3, PR metadata.** The description calls this a draft, but the title has no `WIP:` prefix (`git-and-workflow.md` §2.1, AGENTS.md §4). - **P3, `schemas/statistics.py:176`.** New contract field `EntranceStatistics.zone_name`; CONTEXT.md lists "Zone" under *Avoid* for Camera Group. Follows the existing column, so low priority. ### Baseline smells (judgement calls) - **P3, possible Duplicated Code:** - The comparison block `change_percent=round(100 * (x / ref - 1), 1) if ref else None, reason="ZERO_REFERENCE_VISITORS" if ref == 0 else None` appears four times (`analytics_service.py:181-182, 268-269, 342-343, 364-366`). Extract a single `VisitorComparison` factory. - Trusted-multiplier selection at `:481-491` repeats the logic at `:773-780`. - Each route repeats the same `ValueError→422 / LookupError→404` try/except (`statistics_controller.py:40-47, 58-65`). - **P3, possible Primitive Obsession, `schemas/statistics.py:106`.** `VisitorComparison.reason: str` holds ~seven fixed codes (`NEW_CAMERA`, `NO_DATA_LAST_YEAR`, …), while `attribution` and `status` in the same file use `Literal`. Make `reason` a `Literal`. - **P3, possible Repeated Switches, `analytics_service.py:225-228, 250-300`.** `get_statistics_summary_async` (~110 lines) checks `granularity` in two `!= "day"` conditions then again in an `if/elif/else`. One helper per granularity would shorten it. - **P3, possible Mysterious Name, `schemas/statistics.py:148-149`.** `busiest_day` (week) and `best_day` (month) hold the same value from the same variable. Use one name. - **P3, possible Mysterious Name, `analytics_service.py:334`.** `old`, `prior`, `top` are unclear; `previous_row` / `previous_comparison` would read better. ## Spec Nothing blocks merge. Both routes, every field the mockup lists, `require_auth`, the closed-period check, counted-camera filtering and the triage decisions are in place. ### (a) Missing or partial - **P2 · No tests for several claimed behaviours** (`tests/test_statistics_summary.py`). No test for: - a week summary (`busiest_day`, week weekend share); - any case where last-year data *exists*, for the summary or the entrances route; - ISO-week or Feb-29 last-year alignment; - the `NO_DATA_PREVIOUS_PERIOD` reason; - the 404 when a period has no data; - the `NO_REFERENCE_DATA` status; - dwell weighted by open-window visitors. The spec requires "vs same period last year (only when last year has data…)", but only the omitted branch is tested. - **P3** · Spec: "Each comparison returns `null` with a reason". The code returns a `VisitorComparison` object with `reference_visitors: null` plus `reason`, not a literal `null`. Fine if the deck expects the object; document it. ### (b) Scope creep - **P3 · `analytics_service.py:476-490`**: `/timeseries/daily` peaks now use each cycle's trusted multiplier. Changes #84's route (not asked for here, though sound — and #89's spec review asks for exactly this). `get_dwell_dayparts_async` (`:589`) still uses the active multiplier, so dwell for the same day can differ between the summary and the dayparts route. - **P3 · `:350`**: adds a third status, `NO_REFERENCE_DATA`, beyond the `NEW` rule in the triage. Reasonable, but new contract surface. ### (c) Implemented but questionable - **P2 · Partial-period comparisons** (`:170`, `:216`). Triage: "Partial periods: figures cover only days with data". The current total covers only its covered days, but the reference sum is the raw total of the reference period. A 2-of-30-day month compared with a full month shows roughly −90%. Normalise by covered days or return the reference period's `covered_days`. - **P3 · Cameras with no current flow** (`occupancy_repository.py:1259`, `:331`). Triage: "a camera that disappeared is not listed". An active camera with zero events this period is still listed as `EXISTING` at −100%. If "disappeared" means "no events", this contradicts the rule. Clarify. - **P3 · Last-year detection on entrances** (`:326`). `has_year_data` looks only at *current* cameras. If last year's data came only from cameras since removed, `last_year` is omitted even though the summary route shows it. - **P3** · `tests/test_statistics_summary.py:139`: the unaligned-week check is skipped whenever the test runs on a Monday. ### Open design points | Point | Verdict | |---|---| | New or removed cameras (#86) | Decided and sound: first event ≥ period start means `NEW`; inactive cameras dropped | | Multi-camera estimates (#86) | Decided and sound: `attribution: ESTIMATED` | | Last-year alignment | Decided and sound: ISO week, calendar month, calendar day with Feb 29 → Feb 28 (`:94`). Undocumented | | Partial periods versus comparisons | Decided but questionable (see (c)) | | Weekend share (Saturday and Sunday business days) | Decided and sound | | Usual weekday fewer than 2 samples | Decided and sound: `INSUFFICIENT_USUAL_WEEKDAYS` with `source_dates` | | Mean dwell over the period | Decided and sound: weighting by open-window visitors equals pooled Little's Law | --- **Summary:** Standards 12 (1×P2, 11×P3) — worst: comparisons pit covered-days-only totals against full reference periods. Spec 8 (2×P2, 6×P3) — worst: same partial-period comparison flaw, plus missing tests for last-year-present and week paths. 🤖 Generated with [Claude Code](https://claude.com/claude-code) 
# Conflicts:
#	app/services/analytics_service.py
#	tests/test_statistics_periods.py
# Conflicts:
#	app/db/occupancy_repository.py
# Conflicts:
#	tests/test_statistics_periods.py
gabogg commented 2026-09-25 10:28:21 +00:00
Author
Owner
Copy link

Code review — pass 2 (two-axis)

Re-reviewed at head 028c491. Each axis verified every pass-1 finding against the code (not the commit messages), then reviewed the fix commits for new problems. Verdicts: ✅ fixed · ◐ partial · ❌ not addressed · ↺ regressed · ⊘ declined with documented reason. Per review policy, from pass 2 on P1/P2 block merge; P3s go to one follow-up issue for this PR.

Standards

Head 028c491. ruff check/format clean; statistics tests (10) pass and full suite passes (295 passed, 1 skipped) in a throwaway worktree.

Pass-1 findings

Finding Verdict Evidence
P2 partial-period comparison against a full reference period ✅ fixed fbbec92: statistics_visitor_comparison_async returns PARTIAL_PERIOD with no change_percent when either side is partial (analytics_service.py:180-192); entrances likewise (:374-382, :401-409). Summary path tested (test_statistics_summary.py:198); documented in docs/api/README.md.
P3 is_new ignores period end ✅ fixed fbbec92 :385-388: current_epoch <= first_event < end_epoch
P3 dayparts dwell uses a different multiplier from the daily series ❌ not addressed get_dwell_dayparts_async still uses active_exit_multiplier (:631); daily series uses the cycle's trusted multiplier (:510-524). Undocumented.
P3 WIP: title vs "draft" wording ✅ fixed Description no longer calls it a draft.
P3 zone_name vs CONTEXT.md "Zone" avoid-list ❌ not addressed EntranceStatistics.zone_name unchanged.
P3 Duplicated comparison block ↺ worse The inline change_percent/reason ternary is now a nested three-way ternary with a partial-period check, repeated at :370-382 and :397-409; the helper at :162-198 holds the same rule a third time.
P3 Duplicated trusted-multiplier selection ◐ partial Copies remain and have drifted (see below).
P3 Duplicated controller try/except ❌ not addressed Both routes still repeat ValueError→422 / LookupError→404.
P3 reason: str primitive ◐ partial a7aa664 adds ComparisonReason Literal; StatisticsSummary.previous_month_daily_average_reason is still str | None holding the same code (NO_DATA_PREVIOUS_PERIOD).
P3 Repeated Switches on granularity ❌ not addressed get_statistics_summary_async still branches at :180, :239-243, :262-328.
P3 busiest_day / best_day naming ❌ not addressed :316-318
P3 old / prior / top names ❌ not addressed :363-392, :305

New findings

  • P3 · Merge rewrote #89's trusted-multiplier code; the copies now differ (analytics_service.py:510-524). This PR replaces #89's 1f1c688 version with its own, differing in: calibration lookup window bounds.end vs #89's bounds.next_start (this one arguably more correct), and computed_exit_multiplier is not None vs #89's truthiness test. get_calibration_audit (:836-839) still uses truthiness with next_start, so a trusted multiplier of 0.0 is accepted by the daily series but falls back to default in the audit. Land this version in #89 and extract one _trusted_cycle_multiplier(day, bounds, default) helper, or state in the description that #92 supersedes #89's version.
  • P3 · Partial-period comparison logic in three places (:180-192, :370-382, :397-409). One compare_visitors(current, reference, partial) factory clears this and the pass-1 duplication finding.
  • P3 · Previous-month quality queried twice (:182, :320): get_statistics_period_quality_async runs inside the comparison helper and again for previous_month_daily_average. Return the reference quality from the helper.
  • P3 · Type-checker gap: year_quality is Optional (:356-360, read at :402, :405) as .partial without narrowing. Safe at runtime (guarded by has_year_data), but pyright/mypy would flag it.
  • P3 · Two fixed branches untested: entrance PARTIAL_PERIOD and the new is_new upper bound; the only PARTIAL_PERIOD assertion is on the summary route.

Merge readiness (this axis): Ready. The only P2 is fixed; nothing new at P1/P2. Remaining P3s (including multiplier-selection drift) → follow-up issue.

Spec

Head 028c491; own commits fbbec92, 3560dcf, a7aa664, 028c491 vs pass-1 head 1a3f89b. Throwaway worktree: pytest tests/test_statistics_{summary,periods,baselines}.py 13 passed.

Pass-1 findings

Finding Verdict Evidence
P2: no tests for claimed behaviours ◐ partial 3560dcf/fbbec92 add: week summary (busiest_day, weekend share); last year present on both routes; ISO-week and Feb-29 alignment; NO_DATA_PREVIOUS_PERIOD; the 404. Still untested: NO_REFERENCE_DATA, and period dwell weighted by open_window_visitors (analytics_service.py:215-229).
P3: comparison is an object, not literal null ✅ documented 028c491, docs/api/README.md:132
P3: /timeseries/daily scope creep; dayparts dwell uses a different multiplier ❌ not addressed get_dwell_dayparts_async still uses active_exit_multiplier (analytics_service.py:631); same day can show two dwell values.
P3: extra NO_REFERENCE_DATA status ✅ documented fbbec92, docs/api/README.md:132
P2: partial-period comparisons ✅ fixed (suppress) fbbec92: :182-192 summary, :375-382/:402-409 entrances. Month test checks PARTIAL_PERIOD (test_statistics_summary.py:197-198).
P3: zero-event cameras listed at −100% ✅ fixed, untested fbbec92 :363 (if not row["event_count"]: continue)
P3: has_year_data checks only current cameras ✅ fixed fbbec92 :342-348: whole-period counted flow, matching the summary route.
P3: unaligned-week check skipped on Mondays ✅ fixed fbbec92, test_statistics_summary.py:137-141
Design point: alignment undocumented ✅ fixed docs/api/README.md:132 + unit test test_statistics_summary.py:26

New findings

  • P3: suppress rule is sound, but "partial" is too broad a trigger (:186, :376, :403; partial = covered < business_days at :467). Spec: "Partial periods: figures cover only days with data; the response carries covered_days / business_days." Suppressing is honest — the reference sum is raw and there's no reference covered_days, so the deck couldn't normalise anyway. But a scheduled-closed holiday (occupancy_service.py:235, "Cerrado") with no counted events makes the whole week/month partial, so every month containing a closure loses its previous-period and last-year comparison, and so does the month after it. This rule was agent-decided, not user-decided: file a needs-triage follow-up (exclude scheduled-closed days from business_days, or compare daily averages).
  • P3: new fix paths untested: PARTIAL_PERIOD on the entrances route; skipping zero-event cameras (:363); the new < end_epoch bound on is_new (:387).
  • P3: unaligned-week test uses a future date (test_statistics_summary.py:137): 422 for either reason (not Monday, or not closed), so it doesn't isolate alignment. Use a past Tuesday.

Nothing new at P1/P2. Spec coverage of #83 and #86 remains complete.

Merge readiness (this axis): Ready — no P1/P2. File the P3s plus the dwell-weighting and NO_REFERENCE_DATA test gaps as a follow-up issue.

Summary: Ready on both axes (no P1/P2). Worst per axis: Standards → trusted-multiplier copies drifted between #89 and #92 (P3, resolve when #89 lands); Spec → PARTIAL_PERIOD suppression triggers on scheduled closures (P3, agent-made decision → needs-triage follow-up).

🤖 Generated with Claude Code

# Code review — pass 2 (two-axis) Re-reviewed at head `028c491`. Each axis verified every pass-1 finding against the code (not the commit messages), then reviewed the fix commits for new problems. Verdicts: ✅ fixed · ◐ partial · ❌ not addressed · ↺ regressed · ⊘ declined with documented reason. Per review policy, from pass 2 on **P1/P2 block merge; P3s go to one follow-up issue** for this PR. ## Standards Head 028c491. ruff check/format clean; statistics tests (10) pass and full suite passes (295 passed, 1 skipped) in a throwaway worktree. ### Pass-1 findings | Finding | Verdict | Evidence | |---|---|---| | P2 partial-period comparison against a full reference period | ✅ fixed | fbbec92: `statistics_visitor_comparison_async` returns `PARTIAL_PERIOD` with no `change_percent` when either side is partial (`analytics_service.py:180-192`); entrances likewise (`:374-382`, `:401-409`). Summary path tested (`test_statistics_summary.py:198`); documented in `docs/api/README.md`. | | P3 `is_new` ignores period end | ✅ fixed | fbbec92 `:385-388`: `current_epoch <= first_event < end_epoch` | | P3 dayparts dwell uses a different multiplier from the daily series | ❌ not addressed | `get_dwell_dayparts_async` still uses `active_exit_multiplier` (`:631`); daily series uses the cycle's trusted multiplier (`:510-524`). Undocumented. | | P3 `WIP:` title vs "draft" wording | ✅ fixed | Description no longer calls it a draft. | | P3 `zone_name` vs CONTEXT.md "Zone" avoid-list | ❌ not addressed | `EntranceStatistics.zone_name` unchanged. | | P3 Duplicated comparison block | ↺ worse | The inline `change_percent`/`reason` ternary is now a nested three-way ternary with a partial-period check, repeated at `:370-382` and `:397-409`; the helper at `:162-198` holds the same rule a third time. | | P3 Duplicated trusted-multiplier selection | ◐ partial | Copies remain and have drifted (see below). | | P3 Duplicated controller try/except | ❌ not addressed | Both routes still repeat `ValueError→422 / LookupError→404`. | | P3 `reason: str` primitive | ◐ partial | a7aa664 adds `ComparisonReason` Literal; `StatisticsSummary.previous_month_daily_average_reason` is still `str \| None` holding the same code (`NO_DATA_PREVIOUS_PERIOD`). | | P3 Repeated Switches on `granularity` | ❌ not addressed | `get_statistics_summary_async` still branches at `:180`, `:239-243`, `:262-328`. | | P3 `busiest_day` / `best_day` naming | ❌ not addressed | `:316-318` | | P3 `old` / `prior` / `top` names | ❌ not addressed | `:363-392`, `:305` | ### New findings - **P3 · Merge rewrote #89's trusted-multiplier code; the copies now differ** (`analytics_service.py:510-524`). This PR replaces #89's 1f1c688 version with its own, differing in: calibration lookup window `bounds.end` vs #89's `bounds.next_start` (this one arguably more correct), and `computed_exit_multiplier is not None` vs #89's truthiness test. `get_calibration_audit` (`:836-839`) still uses truthiness with `next_start`, so a trusted multiplier of `0.0` is accepted by the daily series but falls back to default in the audit. Land this version in #89 and extract one `_trusted_cycle_multiplier(day, bounds, default)` helper, or state in the description that #92 supersedes #89's version. - **P3 · Partial-period comparison logic in three places** (`:180-192`, `:370-382`, `:397-409`). One `compare_visitors(current, reference, partial)` factory clears this and the pass-1 duplication finding. - **P3 · Previous-month quality queried twice** (`:182`, `:320`): `get_statistics_period_quality_async` runs inside the comparison helper and again for `previous_month_daily_average`. Return the reference quality from the helper. - **P3 · Type-checker gap: `year_quality` is Optional** (`:356-360`, read at `:402`, `:405`) as `.partial` without narrowing. Safe at runtime (guarded by `has_year_data`), but pyright/mypy would flag it. - **P3 · Two fixed branches untested:** entrance `PARTIAL_PERIOD` and the new `is_new` upper bound; the only `PARTIAL_PERIOD` assertion is on the summary route. **Merge readiness (this axis):** Ready. The only P2 is fixed; nothing new at P1/P2. Remaining P3s (including multiplier-selection drift) → follow-up issue. ## Spec Head 028c491; own commits fbbec92, 3560dcf, a7aa664, 028c491 vs pass-1 head 1a3f89b. Throwaway worktree: `pytest tests/test_statistics_{summary,periods,baselines}.py` 13 passed. ### Pass-1 findings | Finding | Verdict | Evidence | |---|---|---| | P2: no tests for claimed behaviours | ◐ partial | 3560dcf/fbbec92 add: week summary (`busiest_day`, weekend share); last year present on both routes; ISO-week and Feb-29 alignment; `NO_DATA_PREVIOUS_PERIOD`; the 404. Still untested: `NO_REFERENCE_DATA`, and period dwell weighted by `open_window_visitors` (`analytics_service.py:215-229`). | | P3: comparison is an object, not literal `null` | ✅ documented | 028c491, `docs/api/README.md:132` | | P3: `/timeseries/daily` scope creep; dayparts dwell uses a different multiplier | ❌ not addressed | `get_dwell_dayparts_async` still uses `active_exit_multiplier` (`analytics_service.py:631`); same day can show two dwell values. | | P3: extra `NO_REFERENCE_DATA` status | ✅ documented | fbbec92, `docs/api/README.md:132` | | P2: partial-period comparisons | ✅ fixed (suppress) | fbbec92: `:182-192` summary, `:375-382`/`:402-409` entrances. Month test checks `PARTIAL_PERIOD` (`test_statistics_summary.py:197-198`). | | P3: zero-event cameras listed at −100% | ✅ fixed, untested | fbbec92 `:363` (`if not row["event_count"]: continue`) | | P3: `has_year_data` checks only current cameras | ✅ fixed | fbbec92 `:342-348`: whole-period counted flow, matching the summary route. | | P3: unaligned-week check skipped on Mondays | ✅ fixed | fbbec92, `test_statistics_summary.py:137-141` | | Design point: alignment undocumented | ✅ fixed | `docs/api/README.md:132` + unit test `test_statistics_summary.py:26` | ### New findings - **P3: suppress rule is sound, but "partial" is too broad a trigger** (`:186`, `:376`, `:403`; `partial = covered < business_days` at `:467`). Spec: "Partial periods: figures cover only days with data; the response carries `covered_days` / `business_days`." Suppressing is honest — the reference sum is raw and there's no reference `covered_days`, so the deck couldn't normalise anyway. But a scheduled-closed holiday (`occupancy_service.py:235`, "Cerrado") with no counted events makes the whole week/month partial, so every month containing a closure loses its previous-period and last-year comparison, and so does the month after it. This rule was agent-decided, not user-decided: file a `needs-triage` follow-up (exclude scheduled-closed days from `business_days`, or compare daily averages). - **P3: new fix paths untested:** `PARTIAL_PERIOD` on the entrances route; skipping zero-event cameras (`:363`); the new `< end_epoch` bound on `is_new` (`:387`). - **P3: unaligned-week test uses a future date** (`test_statistics_summary.py:137`): 422 for either reason (not Monday, or not closed), so it doesn't isolate alignment. Use a past Tuesday. Nothing new at P1/P2. Spec coverage of #83 and #86 remains complete. **Merge readiness (this axis):** Ready — no P1/P2. File the P3s plus the dwell-weighting and `NO_REFERENCE_DATA` test gaps as a follow-up issue. --- **Summary:** **Ready on both axes** (no P1/P2). Worst per axis: Standards → trusted-multiplier copies drifted between #89 and #92 (P3, resolve when #89 lands); Spec → `PARTIAL_PERIOD` suppression triggers on scheduled closures (P3, agent-made decision → needs-triage follow-up). 🤖 Generated with [Claude Code](https://claude.com/claude-code) 
# Conflicts:
#	app/schemas/statistics.py
#	app/services/analytics_service.py
gabogg changed target branch from feat/statistics-usual-weekday-baselines to master 2026-09-25 12:17:00 +00:00
gabogg merged commit cf20ef2d36 into master 2026-09-25 12:17:29 +00:00
gabogg deleted branch feat/statistics-summary-entrances 2026-09-25 12:17:29 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
blocked
Waits on the issues listed in its "Blocked by:" line before implementation

  
bug
Category: something is broken; behaviour differs from the spec or docs

  
enhancement
Category: new feature or improvement, including refactors, chores and docs

  
high-priority
Address before ordinary-priority work; independent of triage readiness and blockers

  
low-priority
Address after ordinary-priority work; independent of triage readiness and blockers

  
needs-info
Waiting on reporter for more information

  
needs-triage
Maintainer needs to evaluate this issue

  
ready-for-agent
Fully specified, ready for an AFK agent

  
ready-for-human
Requires human implementation

  
referenced
Previously closed issue reopened or referenced by an open issue; visible history marker

  
research
Needs investigation (internet or local resources) before it can be triaged down; used alongside a category and a triage role

  
wontfix
Will not be actioned

No labels
blocked
bug
enhancement
high-priority
low-priority
needs-info
needs-triage
ready-for-agent
ready-for-human
referenced
research
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
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
gabogg/hikcentral!92
No description provided.