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): usual-weekday baselines (#88) #90

Merged
gabogg merged 9 commits from feat/statistics-usual-weekday-baselines into master 2026-09-25 12:17:24 +00:00
Conversation 2 Commits 9 Files changed 9 +652 -17
gabogg commented 2026-09-25 01:06:00 +00:00
Owner
Copy link

Summary

Implements #88's usual-weekday baseline for hourly visitors and open-window dayparts. Also fixes #77 item 2: the admin dayparts route now defaults to the active business day before the reset. This PR is stacked on #89.

Architectural impact

The existing analytics service selects up to four trusted, non-holiday, gap-free matching weekdays from the previous eight weeks. Both admin routes gain baseline=same_weekday_4w; authenticated read-only presenter wrappers live under /api/statistics/. Pydantic responses include source dates, unavailable reasons, and day quality. No schema migration.

Verification

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

Checklist

  • #88: hourly baseline, daypart baseline, provenance dates, insufficient-sample reason.
  • #88: skip holiday, ingestion-gap and untrusted days.
  • #77 item 2: pre-reset dayparts default uses business date.
  • Review and merge #89 before retargeting this PR to master.

Depends on #89. Refs #80, #88, #77.

Review follow-up

Baseline provenance now lists only usable days. Trust follows the current unsuperseded verdict, and dwell averages are weighted by ingress. Volume-based dayparts reject a baseline because each source day has different time cuts. The selector still looks back up to eight weeks to obtain four samples; same_weekday_4w denotes the maximum sample count.

## Summary Implements #88's usual-weekday baseline for hourly visitors and open-window dayparts. Also fixes #77 item 2: the admin dayparts route now defaults to the active business day before the reset. This PR is stacked on #89. ## Architectural impact The existing analytics service selects up to four trusted, non-holiday, gap-free matching weekdays from the previous eight weeks. Both admin routes gain `baseline=same_weekday_4w`; authenticated read-only presenter wrappers live under `/api/statistics/`. Pydantic responses include source dates, unavailable reasons, and day quality. No schema migration. ## Verification - `pytest -q`: full suite passed after review fixes. - Pre-commit: Ruff lint, Ruff format, pytest passed. - Documentation check: 33 Markdown files and 70 HTTP operations. ## Checklist - [x] #88: hourly baseline, daypart baseline, provenance dates, insufficient-sample reason. - [x] #88: skip holiday, ingestion-gap and untrusted days. - [x] #77 item 2: pre-reset dayparts default uses business date. - [ ] Review and merge #89 before retargeting this PR to `master`. Depends on #89. Refs #80, #88, #77. ## Review follow-up Baseline provenance now lists only usable days. Trust follows the current unsuperseded verdict, and dwell averages are weighted by ingress. Volume-based dayparts reject a baseline because each source day has different time cuts. The selector still looks back up to eight weeks to obtain four samples; `same_weekday_4w` denotes the maximum sample count.
gabogg changed title from WIP: feat(statistics): usual-weekday baselines (#88) to feat(statistics): usual-weekday baselines (#88) 2026-09-25 08:31:50 +00:00
gabogg commented 2026-09-25 08:39:16 +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

Bugs and correctness

  • P2 app/services/analytics_service.py:411-442: the dayparts baseline reports provenance that doesn't match what it averaged. usable drops samples that don't have exactly 3 dayparts, but the response still sends dates=dates (every candidate day), which defeats #88's provenance requirement. Return only the dates actually used. Also, mean_share_percent averages over samples where total > 0, while mean_visitors and mean_dwell_minutes average over all of usable, so the three figures can come from different sample sets.
  • P2 app/db/occupancy_repository.py:1270-1281: is_cycle_data_trusted_async only reads AUTOMATIC_NOCTURNAL rows and has no superseded_by IS NULL check. The existing trust selection (~line 1685) ranks RETROACTIVE_GUARD_AUDIT and manual verdicts above the automatic one, so the two trust checks can disagree about the same cycle. Resolve or document.

Documented-standard issues

  • P2 app/services/analytics_service.py:650-685: the hourly baseline is built as untyped nested dicts even though UsualHourlyBaseline and PresenterHourlyResponse exist in this PR. code-standards.md §2.3: "Avoid passing untyped, raw dictionaries … when structured schemas are available". The dayparts path builds Pydantic models for the same thing. (Judgement: the surrounding function already returns a dict.)
  • P2 tests/test_statistics_baselines.py: code-standards.md §4.3 requires tests for every new endpoint/fix. Tests check source dates and the #77 default, but not: the INSUFFICIENT_USUAL_WEEKDAYS reason; the averaged values (mean_visitors, shares); the 422 cases (open day, view=week, bucket_minutes not 15/30/60).
  • P3 git-and-workflow.md §2.1 / AGENTS.md §4: the description calls this a draft, but the title has no WIP: prefix.

Baseline smells (all judgement calls)

  • P3 Duplicated Code in analytics_service.py: datetime.date.fromisoformat(facility_cycle_bounds(None, reset).label) is newly repeated at lines 93, 120 and ~306 (already at 168 and 205). Extract an active_business_day(reset) helper. The config-read-then-reset preamble in validate_closed_statistics_day_async repeats the callers' config read, so every presenter request reads config twice.
  • P3 Duplicated Code in controllers: the same except ValueError → HTTPException(422, VALIDATION_ERROR) block at statistics_controller.py:60,75 and analytics_controller.py:40,99. One shared helper or exception handler.
  • P3 Primitive Obsession: controllers type baseline as Literal["same_weekday_4w"], but the service takes baseline: str | None (~300, ~452), and reason is a bare string. Share one Literal/enum alias in app/schemas/.
  • P3 Feature Envy / N+1: each baseline sample re-runs full get_hourly_timeseries_async / get_dwell_dayparts_async (~408, ~660), re-running cycle-quality and config queries per date. Acceptable for 4 dates; a narrower per-day aggregate would be clearer.
  • P3 Mysterious Name: same_weekday_4w promises "4 weeks", but the search goes back 8 weeks (lines 89-115 and API docs). It means "up to 4 samples from 8 weeks".
  • P3 Mysterious Name: comprehension variable date (~409) shadows-in-reading the date type; rename to sample_day.

Spec

Overall the PR matches the spec. Both admin routes take baseline=same_weekday_4w. The login-only wrappers live under /api/statistics/, the response lists the source dates, and fewer than two usable days returns null with a reason. The #77 item 2 default is fixed.

(a) Missing or partial

  • P2: tests don't check read-only access for non-admins. Spec: "Access: any logged-in user, read-only (require_auth)." tests/test_statistics_baselines.py only logs in as admin, so nothing proves a viewer can reach /api/statistics/timeseries/hourly and /dwell/dayparts.
  • P2: the "no baseline" path has no test. Spec: "With fewer than 2 usable days there is no baseline (null plus a reason)." The INSUFFICIENT_USUAL_WEEKDAYS branches (analytics_service.py:404, :414) are never run. The daypart baseline test checks only dates, not mean_visitors, mean_dwell_minutes or mean_share_percent.
  • P3: several rejections have no test. Nothing covers the closed-day rejection on the deck routes (analytics_service.py:117), view=week with a baseline, or unsupported bucket sizes (:650).

(b) Scope creep

  • None of substance. Adding gap_estimated, data_trust and cycle_completeness to both responses follows the shared constraint "Every period-level figure carries its data quality".

(c) Implemented but looks wrong

  • P3: some listed dates may not be in the average. Spec: "The response lists which dates formed the baseline." At analytics_service.py:411, samples without three dayparts are dropped, but dates still lists every selected day.
  • P3: the usual dwell is a plain average. mean_dwell_minutes (:431) averages each day's dwell equally. Dayparts with count_in == 0 add 0.0, and busy days get no extra weight. This clashes with the fix in 1a3f89b (PR #92), which weights dwell by open-window visitors.
  • P3: volume-mode dayparts are averaged by position only. In mode=volume each day has its own daypart cut times, so dayparts are averaged by index alone. UsualDaypart carries no label, so the deck can't show which times the usual split covers.

Open design points

Point Verdict
Skip holiday, gap-estimated and low-trust days ("should be skipped (or reported)") Decided and sound: skipped, lookback up to 8 weeks.
What counts as "low Data Trust" Decided but questionable. is_cycle_data_trusted_async (occupancy_repository.py:1270) uses the is_trusted verdict, not data_trust_score. A MANUAL_ADMIN audit forces it to true (occupancy_service.py:1098). A day with no audit row is silently excluded. None of this is documented.
Days with no counted flow Decided and sound: skipped (analytics_service.py:105).
#77 item 2 business-day default Decided and sound, with a test.
Bucket sizes allowed with a baseline Decided and sound: 15, 30 and 60 minutes only, otherwise 422.

Summary: Standards 11 (4×P2, 7×P3) — worst: daypart baseline dates lists days that weren't averaged. Spec 6 (2×P2, 4×P3) + 1 questionable design point — worst: no test for the INSUFFICIENT_USUAL_WEEKDAYS path or non-admin access.

🤖 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 ### Bugs and correctness - **P2** `app/services/analytics_service.py:411-442`: the dayparts baseline reports provenance that doesn't match what it averaged. `usable` drops samples that don't have exactly 3 dayparts, but the response still sends `dates=dates` (every candidate day), which defeats #88's provenance requirement. Return only the dates actually used. Also, `mean_share_percent` averages over samples where `total > 0`, while `mean_visitors` and `mean_dwell_minutes` average over all of `usable`, so the three figures can come from different sample sets. - **P2** `app/db/occupancy_repository.py:1270-1281`: `is_cycle_data_trusted_async` only reads `AUTOMATIC_NOCTURNAL` rows and has no `superseded_by IS NULL` check. The existing trust selection (~line 1685) ranks `RETROACTIVE_GUARD_AUDIT` and manual verdicts above the automatic one, so the two trust checks can disagree about the same cycle. Resolve or document. ### Documented-standard issues - **P2** `app/services/analytics_service.py:650-685`: the hourly baseline is built as untyped nested dicts even though `UsualHourlyBaseline` and `PresenterHourlyResponse` exist in this PR. code-standards.md §2.3: "Avoid passing untyped, raw dictionaries … when structured schemas are available". The dayparts path builds Pydantic models for the same thing. (Judgement: the surrounding function already returns a dict.) - **P2** `tests/test_statistics_baselines.py`: code-standards.md §4.3 requires tests for every new endpoint/fix. Tests check source `dates` and the #77 default, but not: the `INSUFFICIENT_USUAL_WEEKDAYS` reason; the averaged values (`mean_visitors`, shares); the 422 cases (open day, `view=week`, `bucket_minutes` not 15/30/60). - **P3** git-and-workflow.md §2.1 / AGENTS.md §4: the description calls this a draft, but the title has no `WIP:` prefix. ### Baseline smells (all judgement calls) - **P3 Duplicated Code** in `analytics_service.py`: `datetime.date.fromisoformat(facility_cycle_bounds(None, reset).label)` is newly repeated at lines 93, 120 and ~306 (already at 168 and 205). Extract an `active_business_day(reset)` helper. The config-read-then-`reset` preamble in `validate_closed_statistics_day_async` repeats the callers' config read, so every presenter request reads config twice. - **P3 Duplicated Code** in controllers: the same `except ValueError → HTTPException(422, VALIDATION_ERROR)` block at `statistics_controller.py:60,75` and `analytics_controller.py:40,99`. One shared helper or exception handler. - **P3 Primitive Obsession**: controllers type `baseline` as `Literal["same_weekday_4w"]`, but the service takes `baseline: str | None` (~300, ~452), and `reason` is a bare string. Share one `Literal`/enum alias in `app/schemas/`. - **P3 Feature Envy / N+1**: each baseline sample re-runs full `get_hourly_timeseries_async` / `get_dwell_dayparts_async` (~408, ~660), re-running cycle-quality and config queries per date. Acceptable for 4 dates; a narrower per-day aggregate would be clearer. - **P3 Mysterious Name**: `same_weekday_4w` promises "4 weeks", but the search goes back 8 weeks (lines 89-115 and API docs). It means "up to 4 samples from 8 weeks". - **P3 Mysterious Name**: comprehension variable `date` (~409) shadows-in-reading the `date` type; rename to `sample_day`. ## Spec Overall the PR matches the spec. Both admin routes take `baseline=same_weekday_4w`. The login-only wrappers live under `/api/statistics/`, the response lists the source dates, and fewer than two usable days returns `null` with a reason. The #77 item 2 default is fixed. ### (a) Missing or partial - **P2: tests don't check read-only access for non-admins.** Spec: *"Access: any logged-in user, read-only (`require_auth`)."* `tests/test_statistics_baselines.py` only logs in as `admin`, so nothing proves a viewer can reach `/api/statistics/timeseries/hourly` and `/dwell/dayparts`. - **P2: the "no baseline" path has no test.** Spec: *"With fewer than 2 usable days there is no baseline (`null` plus a reason)."* The `INSUFFICIENT_USUAL_WEEKDAYS` branches (`analytics_service.py:404`, `:414`) are never run. The daypart baseline test checks only `dates`, not `mean_visitors`, `mean_dwell_minutes` or `mean_share_percent`. - **P3: several rejections have no test.** Nothing covers the closed-day rejection on the deck routes (`analytics_service.py:117`), `view=week` with a baseline, or unsupported bucket sizes (`:650`). ### (b) Scope creep - None of substance. Adding `gap_estimated`, `data_trust` and `cycle_completeness` to both responses follows the shared constraint *"Every period-level figure carries its data quality"*. ### (c) Implemented but looks wrong - **P3: some listed dates may not be in the average.** Spec: *"The response lists which dates formed the baseline."* At `analytics_service.py:411`, samples without three dayparts are dropped, but `dates` still lists every selected day. - **P3: the usual dwell is a plain average.** `mean_dwell_minutes` (`:431`) averages each day's dwell equally. Dayparts with `count_in == 0` add `0.0`, and busy days get no extra weight. This clashes with the fix in 1a3f89b (PR #92), which weights dwell by open-window visitors. - **P3: volume-mode dayparts are averaged by position only.** In `mode=volume` each day has its own daypart cut times, so dayparts are averaged by index alone. `UsualDaypart` carries no label, so the deck can't show which times the usual split covers. ### Open design points | Point | Verdict | |---|---| | Skip holiday, gap-estimated and low-trust days (*"should be skipped (or reported)"*) | Decided and sound: skipped, lookback up to 8 weeks. | | What counts as "low Data Trust" | Decided but questionable. `is_cycle_data_trusted_async` (`occupancy_repository.py:1270`) uses the `is_trusted` verdict, not `data_trust_score`. A `MANUAL_ADMIN` audit forces it to true (`occupancy_service.py:1098`). A day with no audit row is silently excluded. None of this is documented. | | Days with no counted flow | Decided and sound: skipped (`analytics_service.py:105`). | | #77 item 2 business-day default | Decided and sound, with a test. | | Bucket sizes allowed with a baseline | Decided and sound: 15, 30 and 60 minutes only, otherwise 422. | --- **Summary:** Standards 11 (4×P2, 7×P3) — worst: daypart baseline `dates` lists days that weren't averaged. Spec 6 (2×P2, 4×P3) + 1 questionable design point — worst: no test for the `INSUFFICIENT_USUAL_WEEKDAYS` path or non-admin access. 🤖 Generated with [Claude Code](https://claude.com/claude-code) 
# Conflicts:
#	app/controllers/statistics_controller.py
#	app/db/occupancy_repository.py
# Conflicts:
#	app/db/occupancy_repository.py
gabogg commented 2026-09-25 10:28:20 +00:00
Author
Owner
Copy link

Code review — pass 2 (two-axis)

Re-reviewed at head 22490be. 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

Scope: fix commits b414ad3, 1d34b1f, 22490be vs pass-1 head f474b12, plus a re-read of the full own diff. tests/test_statistics_baselines.py passes in a throwaway worktree; ruff check clean.

Pass-1 findings

Finding Verdict Evidence
P2: daypart dates listed days not averaged; share used a different sample set ✅ fixed b414ad3, analytics_service.py:427-433: usable requires 3 parts and entries > 0; used_dates returned in both branches. Shares, visitors, dwell all average over usable.
P2: is_cycle_data_trusted_async read only the automatic verdict, skipped superseded_by ✅ fixed occupancy_repository.py:1311-1325 filters is_current = 1 AND superseded_by IS NULL, same priority order as the trusted-history query at :1729. Documented in docs/api/README.md.
P2: hourly baseline built as untyped dicts ✅ fixed analytics_service.py:679-709 builds UsualHourlyBaseline/UsualHourlyBucket, then .model_dump(mode="json").
P2: missing tests (reason, averages, 422s) ◐ partial Now tested: insufficient-sample reason, closed day, view=week, bucket_minutes=20, mode=volume, operator access; hourly mean_visitors and daypart visitor/share sums asserted. Not covered: mean_dwell_minutes (new weighted dwell) and the new trust-priority rule (manual/retroactive verdicts, superseded rows).
P3: WIP: title vs "draft" wording ✅ fixed Description no longer calls it a draft.
P3: repeated active-business-day expression / double config read ❌ not addressed analytics_service.py:96, 123, 313.
P3: repeated ValueError → 422 blocks in controllers ❌ not addressed Now three copies in statistics_controller.py (40, 62, 77).
P3: baseline: str | None and bare-string reason ❌ not addressed Service signatures unchanged; schemas/statistics.py:78 still reason: str | None.
P3: each sample re-runs the full day computation ❌ not addressed Acceptable for 4 samples.
P3: same_weekday_4w suggests 4 weeks ⊘ declined Description: "denotes the maximum sample count"; docs say "preceding eight weeks".
P3: comprehension variable date ✅ fixed Renamed sample_day (b414ad3).

New findings

  • P2 app/controllers/statistics_controller.py:51 (added in b414ad3): viewer-facing bucket_minutes: int = Query(default=60) has no bounds or allowed set; the service only validates it when baseline is set (analytics_service.py:676). Reproduced in the worktree as an operator:

    • bucket_minutes=0 with calibration_mode='FLOW_RATE_DENSITY' → 500 (ZeroDivisionError at analytics_service.py:625).
    • bucket_minutes=-5 → 200 with 1440 one-minute buckets, response reporting bucket_minutes: -5.

    Breaks AGENTS.md §2 ("Validate input strictly") and the {detail, error_code} error contract. Type it Literal[15, 30, 60] (or Literal[15, 30, 60, 1440] without a baseline).

  • P3 occupancy_repository.py:1318-1321 and :1729-1731: the verdict-priority CASE is written twice and must change in step with the ranking at :2055 (Duplicated Code). Extract one SQL fragment/constant.

  • P3 analytics_service.py:462-464: if shares else 0.0 is now dead (usable always has ≥2 entries with total > 0). Remove.

  • P3 tests/test_statistics_baselines.py:111-112: sum of 1-decimal-rounded values compared with == 100.0; fragile. Use pytest.approx(100.0, abs=0.2).

  • P3 analytics_service.py:676: bucket-size check runs after the target day's full bucket computation; validate first.

Merge readiness (this axis): Blocked by one new P2 (unbounded viewer bucket_minutes → 500) and the partial P2 on tests (weighted dwell, trust priority). P3s → follow-up issue.

Spec

Checked b414ad3, 1d34b1f, 22490be against the branch head. tests/test_statistics_baselines.py passes in a throwaway worktree.

Pass-1 findings

Finding Verdict Evidence
P2: no test that non-admins get read-only access ✅ fixed 1d34b1f, test lines 197-205: operator (non-admin) gets 200 on both /api/statistics/ routes.
P2: no-baseline path untested; daypart means not checked ◐ partial Line 143 covers the first INSUFFICIENT_USUAL_WEEKDAYS branch (no candidates, dates == []). The second branch — candidates exist but fewer than 2 usable — is never run. Lines 101/111 check mean_visitors (47.5). The share check (112) always holds because every seeded event is at 10:00 (one daypart at 100%). mean_dwell_minutes not asserted.
P3: rejections untested ✅ fixed Lines 162-195: closed day, view=week, bucket_minutes=20, mode=volume.
P3: listed dates vs averaged dates ✅ fixed b414ad3: used_dates built from usable; mean_share_percent uses the same set.
P3: plain-average dwell ✅ fixed b414ad3: weighted by ingress, Σ(dwell·in) / Σin.
P3: volume dayparts averaged by position ✅ fixed by rejecting 1d34b1f: baseline + mode == "volume" → 422; rule in docs/api/README.md.
Design point: what counts as "low Data Trust" ✅ decided and documented b414ad3, is_cycle_data_trusted_async: current unsuperseded verdict, ranked retroactive audit > manual > automatic. README: days with no current verdict or an untrusted one are excluded.

New findings

  • P2: glossary term missing. Spec: "Usual Weekday Baseline (glossary): mean of the previous 4 same weekdays…". CONTEXT.md has no such entry. The selection rules (trusted verdict, up to 8 weeks back, ≥2 days, equal dayparts only) live only in the API README.
  • P3: "equal clock segments" overstates the guarantee. In get_dwell_dayparts_async, equal edges come from each day's own open_epoch/close_epoch. If opening hours changed during the 8-week lookback, days are still averaged by position with different cut times — the problem the volume-mode rejection solves. UsualDaypart still has no label. README should say "equal thirds of each day's open window", or drop source days whose edges differ from the target day's.
  • P3: test sends the wrong parameter name. Lines 170 and 189 call /api/analytics/dwell/dayparts with date=, but the route parameter is day (analytics_controller.py:30). Passes only because the 422 check runs before day is used.
  • P3: hourly baseline has no guard for differing buckets. Averages by bucket_time_label across days without a check like the daypart one. Fine for fixed clock buckets; noted for completeness.

Other spec points check out: listed dates = averaged dates; holiday/gap/untrusted days skipped; <2 usable days → null + reason; #77 item 2 fixed; controllers thin.

Merge readiness (this axis): Not yet — one P2 (Usual Weekday Baseline glossary entry in CONTEXT.md). Everything else is P3 → follow-up issue.

Summary: Blocking (P2): Standards — viewer bucket_minutes unbounded (0 → 500 ZeroDivisionError, negatives accepted), plus weighted-dwell / trust-priority tests missing; Spec — no Usual Weekday Baseline glossary entry in CONTEXT.md. Worst per axis: Standards → the 500; Spec → missing glossary term.

🤖 Generated with Claude Code

# Code review — pass 2 (two-axis) Re-reviewed at head `22490be`. 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 Scope: fix commits b414ad3, 1d34b1f, 22490be vs pass-1 head f474b12, plus a re-read of the full own diff. `tests/test_statistics_baselines.py` passes in a throwaway worktree; `ruff check` clean. ### Pass-1 findings | Finding | Verdict | Evidence | |---|---|---| | P2: daypart `dates` listed days not averaged; share used a different sample set | ✅ fixed | b414ad3, `analytics_service.py:427-433`: `usable` requires 3 parts and entries > 0; `used_dates` returned in both branches. Shares, visitors, dwell all average over `usable`. | | P2: `is_cycle_data_trusted_async` read only the automatic verdict, skipped `superseded_by` | ✅ fixed | `occupancy_repository.py:1311-1325` filters `is_current = 1 AND superseded_by IS NULL`, same priority order as the trusted-history query at `:1729`. Documented in `docs/api/README.md`. | | P2: hourly baseline built as untyped dicts | ✅ fixed | `analytics_service.py:679-709` builds `UsualHourlyBaseline`/`UsualHourlyBucket`, then `.model_dump(mode="json")`. | | P2: missing tests (reason, averages, 422s) | ◐ partial | Now tested: insufficient-sample reason, closed day, `view=week`, `bucket_minutes=20`, `mode=volume`, operator access; hourly `mean_visitors` and daypart visitor/share sums asserted. Not covered: `mean_dwell_minutes` (new weighted dwell) and the new trust-priority rule (manual/retroactive verdicts, superseded rows). | | P3: `WIP:` title vs "draft" wording | ✅ fixed | Description no longer calls it a draft. | | P3: repeated active-business-day expression / double config read | ❌ not addressed | `analytics_service.py:96, 123, 313`. | | P3: repeated `ValueError` → 422 blocks in controllers | ❌ not addressed | Now three copies in `statistics_controller.py` (40, 62, 77). | | P3: `baseline: str \| None` and bare-string `reason` | ❌ not addressed | Service signatures unchanged; `schemas/statistics.py:78` still `reason: str \| None`. | | P3: each sample re-runs the full day computation | ❌ not addressed | Acceptable for 4 samples. | | P3: `same_weekday_4w` suggests 4 weeks | ⊘ declined | Description: "denotes the maximum sample count"; docs say "preceding eight weeks". | | P3: comprehension variable `date` | ✅ fixed | Renamed `sample_day` (b414ad3). | ### New findings - **P2** `app/controllers/statistics_controller.py:51` (added in b414ad3): viewer-facing `bucket_minutes: int = Query(default=60)` has no bounds or allowed set; the service only validates it when `baseline` is set (`analytics_service.py:676`). Reproduced in the worktree as an operator: - `bucket_minutes=0` with `calibration_mode='FLOW_RATE_DENSITY'` → **500** (`ZeroDivisionError` at `analytics_service.py:625`). - `bucket_minutes=-5` → 200 with 1440 one-minute buckets, response reporting `bucket_minutes: -5`. Breaks AGENTS.md §2 ("Validate input strictly") and the `{detail, error_code}` error contract. Type it `Literal[15, 30, 60]` (or `Literal[15, 30, 60, 1440]` without a baseline). - **P3** `occupancy_repository.py:1318-1321` and `:1729-1731`: the verdict-priority `CASE` is written twice and must change in step with the ranking at `:2055` (Duplicated Code). Extract one SQL fragment/constant. - **P3** `analytics_service.py:462-464`: `if shares else 0.0` is now dead (`usable` always has ≥2 entries with total > 0). Remove. - **P3** `tests/test_statistics_baselines.py:111-112`: sum of 1-decimal-rounded values compared with `== 100.0`; fragile. Use `pytest.approx(100.0, abs=0.2)`. - **P3** `analytics_service.py:676`: bucket-size check runs after the target day's full bucket computation; validate first. **Merge readiness (this axis):** Blocked by one new P2 (unbounded viewer `bucket_minutes` → 500) and the partial P2 on tests (weighted dwell, trust priority). P3s → follow-up issue. ## Spec Checked b414ad3, 1d34b1f, 22490be against the branch head. `tests/test_statistics_baselines.py` passes in a throwaway worktree. ### Pass-1 findings | Finding | Verdict | Evidence | |---|---|---| | P2: no test that non-admins get read-only access | ✅ fixed | 1d34b1f, test lines 197-205: `operator` (non-admin) gets 200 on both `/api/statistics/` routes. | | P2: no-baseline path untested; daypart means not checked | ◐ partial | Line 143 covers the first `INSUFFICIENT_USUAL_WEEKDAYS` branch (no candidates, `dates == []`). The second branch — candidates exist but fewer than 2 usable — is never run. Lines 101/111 check `mean_visitors` (47.5). The share check (112) always holds because every seeded event is at 10:00 (one daypart at 100%). `mean_dwell_minutes` not asserted. | | P3: rejections untested | ✅ fixed | Lines 162-195: closed day, `view=week`, `bucket_minutes=20`, `mode=volume`. | | P3: listed dates vs averaged dates | ✅ fixed | b414ad3: `used_dates` built from `usable`; `mean_share_percent` uses the same set. | | P3: plain-average dwell | ✅ fixed | b414ad3: weighted by ingress, Σ(dwell·in) / Σin. | | P3: volume dayparts averaged by position | ✅ fixed by rejecting | 1d34b1f: `baseline` + `mode == "volume"` → 422; rule in `docs/api/README.md`. | | Design point: what counts as "low Data Trust" | ✅ decided and documented | b414ad3, `is_cycle_data_trusted_async`: current unsuperseded verdict, ranked retroactive audit > manual > automatic. README: days with no current verdict or an untrusted one are excluded. | ### New findings - **P2: glossary term missing.** Spec: *"**Usual Weekday Baseline** (glossary): mean of the previous 4 same weekdays…"*. `CONTEXT.md` has no such entry. The selection rules (trusted verdict, up to 8 weeks back, ≥2 days, equal dayparts only) live only in the API README. - **P3: "equal clock segments" overstates the guarantee.** In `get_dwell_dayparts_async`, equal edges come from each day's own `open_epoch`/`close_epoch`. If opening hours changed during the 8-week lookback, days are still averaged by position with different cut times — the problem the volume-mode rejection solves. `UsualDaypart` still has no label. README should say "equal thirds of each day's open window", or drop source days whose edges differ from the target day's. - **P3: test sends the wrong parameter name.** Lines 170 and 189 call `/api/analytics/dwell/dayparts` with `date=`, but the route parameter is `day` (`analytics_controller.py:30`). Passes only because the 422 check runs before `day` is used. - **P3: hourly baseline has no guard for differing buckets.** Averages by `bucket_time_label` across days without a check like the daypart one. Fine for fixed clock buckets; noted for completeness. Other spec points check out: listed dates = averaged dates; holiday/gap/untrusted days skipped; <2 usable days → `null` + reason; #77 item 2 fixed; controllers thin. **Merge readiness (this axis):** Not yet — one P2 (Usual Weekday Baseline glossary entry in `CONTEXT.md`). Everything else is P3 → follow-up issue. --- **Summary:** **Blocking (P2):** Standards — viewer `bucket_minutes` unbounded (`0` → 500 `ZeroDivisionError`, negatives accepted), plus weighted-dwell / trust-priority tests missing; Spec — no **Usual Weekday Baseline** glossary entry in `CONTEXT.md`. Worst per axis: Standards → the 500; Spec → missing glossary term. 🤖 Generated with [Claude Code](https://claude.com/claude-code) 
# Conflicts:
#	app/db/occupancy_repository.py
Both hourly routes now accept only 15, 30, 60 or 1440-minute buckets, so
`bucket_minutes=0` no longer raises a 500 and negative sizes are rejected
with VALIDATION_ERROR. Adds tests for the trust-verdict priority used to
pick usual weekdays and for entry-weighted daypart dwell, and records the
Usual Weekday Baseline in the CONTEXT.md glossary.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gabogg changed target branch from feat/statistics-periods-daily to master 2026-09-25 12:16:59 +00:00
gabogg merged commit d2f8bd24be into master 2026-09-25 12:17:24 +00:00
gabogg deleted branch feat/statistics-usual-weekday-baselines 2026-09-25 12:17:25 +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!90
No description provided.