feat(statistics): closed periods and daily series (#82, #84) #89

Merged

gabogg merged 5 commits from feat/statistics-periods-daily into master 2026-09-25 12:17:17 +00:00

Conversation 2 Commits 5 Files changed 7 +711

gabogg commented 2026-09-25 00:57:22 +00:00

Owner

Copy link

Summary

Implements #82 and #84 so the statistics deck can list selectable closed business periods and read daily totals, including the hourly week heatmap data absorbed from #87. PR #80 is blocked on these contracts.

Architectural impact

Adds an authenticated read-only /api/statistics/ router with Pydantic response models. AnalyticsService owns period and daily aggregation; OccupancyRepository owns counted-camera, audit-quality, and gap queries. No schema migration or application write path is added.

Verification

• pytest -q: full suite passed after review fixes.
• Pre-commit: Ruff lint, Ruff format, and pytest passed.
• python3 scripts/check_docs.py: 33 Markdown files and 68 HTTP operations.

Checklist

• #82: closed day/week/month picker rows, partial coverage and quality markers.
• #84: counted-camera daily rows, open-window dwell, peak, holidays, and quality.
• #87: bucket=hour within seven-day ranges, gap-estimated cells.
• Review API response shape and facility-time edge cases.

Refs #80, #82, #84, #87.

Review follow-up

Picker labels are now formatted by the client from structured dates and coverage. Period quality uses one range query per period, and reports excluded cycles. Daily rows use their historical trusted multiplier and expose the occupancy cutover. Regression tests cover peak, dwell, audit scores, day/month boundaries, and limits.

## Summary Implements #82 and #84 so the statistics deck can list selectable closed business periods and read daily totals, including the hourly week heatmap data absorbed from #87. PR #80 is blocked on these contracts. ## Architectural impact Adds an authenticated read-only `/api/statistics/` router with Pydantic response models. `AnalyticsService` owns period and daily aggregation; `OccupancyRepository` owns counted-camera, audit-quality, and gap queries. No schema migration or application write path is added. ## Verification - `pytest -q`: full suite passed after review fixes. - Pre-commit: Ruff lint, Ruff format, and pytest passed. - `python3 scripts/check_docs.py`: 33 Markdown files and 68 HTTP operations. ## Checklist - [x] #82: closed day/week/month picker rows, partial coverage and quality markers. - [x] #84: counted-camera daily rows, open-window dwell, peak, holidays, and quality. - [x] #87: `bucket=hour` within seven-day ranges, gap-estimated cells. - [x] Review API response shape and facility-time edge cases. Refs #80, #82, #84, #87. ## Review follow-up Picker labels are now formatted by the client from structured dates and coverage. Period quality uses one range query per period, and reports excluded cycles. Daily rows use their historical trusted multiplier and expose the occupancy cutover. Regression tests cover peak, dwell, audit scores, day/month boundaries, and limits.

gabogg added 1 commit 2026-09-25 00:57:22 +00:00

feat(statistics): expose closed periods and daily series (#82, #84) 6c64ab647d

gabogg referenced this pull request 2026-09-25 01:06:00 +00:00

feat(statistics): usual-weekday baselines (#88) #90

gabogg referenced this pull request 2026-09-25 01:15:54 +00:00

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

gabogg referenced this pull request 2026-09-25 01:20:45 +00:00

docs(statistics-deck): zero-scroll display model and the projector paradox #80

gabogg changed title from WIP: feat(statistics): closed periods and daily series (#82, #84) to feat(statistics): closed periods and daily series (#82, #84) 2026-09-25 08:31:48 +00:00

gabogg commented 2026-09-25 08:39:15 +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 layering is clean. SQL stays in OccupancyRepository, the controller is thin, contracts sit in app/schemas/statistics.py, and tests cover the 422 and hourly paths. No hard breach of a documented standard.

P2: should fix before merge

• Server-built English labels bypass i18n (judgement call). app/services/analytics_service.py:135-144 formats labels with strftime("%a %d %b %Y").upper() and appends f" ({covered} of {business} days)". CONTEXT.md:224 says the UI has "full dual-language support (English / Spanish) with dynamic client-side switching". Labels here are always English, and %a/%b depend on the process locale, so on the Windows prod host output can change with the service account's locale. Return structured fields (start, end, covered_days) and let the client format the label.
• Query count per request is high (Duplicated Code plus performance, judgement call). analytics_service.py:92-103 and :172-184 repeat the same per-day loop: facility_cycle_bounds_for_date → get_counted_day_flow_async → get_cycle_quality_async. With granularity=month&limit=24 that is ~730 days × 2 queries, plus every empty period back to first_day. Move the shared shape into one range-based "cycle summary" helper, ideally one grouped repository query.

P3: minor

• Primitive Obsession / inconsistent typing. analytics_service.py:161 declares bucket: str = "day", while the controller and DailySeriesResponse use Literal["day", "hour"]. Add a Bucket = Literal[...] alias in schemas/statistics.py, like Granularity.
• Repeated Switches. Three module functions switch on granularity (analytics_service.py:36-66) and the label cascade at :135-142 switches again. A small per-granularity strategy map would keep them together.
• Duplicated overlap predicate. The gap-overlap rule is in SQL (get_cycle_quality_async: start_epoch < ? AND end_epoch >= ?) and again in Python (analytics_service.py:228-232); they can drift.
• Redundant loop guard. analytics_service.py:131: start <= active_day is always true, since start begins at the latest closed period and only moves backwards.
• Status literal. statistics_controller.py:34 uses status_code=422; docs/standards/code-standards.md §3.1 example uses status.HTTP_* constants.
• Config default repeated. cfg.get("daily_reset_time", "04:00") duplicated at analytics_service.py:123 and :164 (possible Data Clumps: cfg/reset travel together).
• Mysterious Name. get_counted_day_flow_async returns an unnamed (int, int, bool) tuple and get_cycle_quality_async returns (bool, int | None, int | None). A NamedTuple each would make call sites readable.

No findings

docs/api/README.md, app/main.py, and the PR description (follows git-and-workflow.md §2 template).

Spec

Overall this matches the triage decisions: separate /api/statistics/ router, require_auth, closed periods only, the counted-camera rule, bucket=hour instead of a new route, no compare=, and the 7-day and 62-day limits. Quarantined events live in a separate table, so counted-camera queries already exclude them.

(a) Missing or partial

• P2: #82 triage: "Bad-data periods (gap estimates, low Data Trust, excluded cycles) are included with their quality fields so the picker can mark them." PeriodQuality has no field for excluded cycles, and get_cycle_quality_async (app/db/occupancy_repository.py:1239) never reads is_trusted or trust_status = 'AUTO_EXCLUDED'. The picker cannot mark an excluded cycle.
• P3: ADR 0006: "analytics responses expose [occupancy_definition_cutover_at] for chart annotation." The daily series does not expose it, so a 62-day range crossing the cutover gets no annotation.

(b) Scope creep

Nothing significant. min_cycle_completeness goes beyond the sketch but follows from "Data Trust and Cycle Completeness for the cycles involved". end is exclusive, as in the sketch.

(c) Implemented but looks wrong

• P2: app/services/analytics_service.py:170. Closed historical days use today's active_exit_multiplier for peak and dwell. Master's timeseries path uses the cycle's logged computed_exit_multiplier, so the same day can show different peak/dwell on the two surfaces — against ADR 0006's point that past calibration is not reconstructible. Commit 1a3f89b fixes it in stacked PR #92. Pull that fix into this PR, or state in the description that #89 is not correct without it.
• P3: app/schemas/statistics.py:15. The sketch shows "min_data_trust": 0.91, but the code returns an int 0–100. Sound choice, not documented in docs/api/README.md.
• P3: analytics_service.py:231. item.end_epoch >= hour_start marks the next hour as gap-estimated when a gap ends exactly on the hour boundary.

Open design points

Point	Verdict
#87 as bucket=hour vs its own route	decided-and-sound
Access (require_auth, separate router)	decided-and-sound
partial = covered_days < business_days, label "N of M days"	decided-and-sound
Week label + iso_week field	decided-and-sound
Range limits → VALIDATION_ERROR	decided-and-sound (:168)
Excluded cycles in period quality	not addressed
Multiplier for closed cycles	decided-but-questionable (see c)
Data Trust scale	decided-but-questionable (not documented)

Test coverage (tests/test_statistics_periods.py)

Covered: 401 without login, excluded camera ignored, partial week with label, open-cycle rejection with error_code, gap marking on hourly cells, 7-day hourly limit.

P2, not tested:

• peak and its time, open-window dwell, holiday flag, data_trust / cycle_completeness values (the #84 row contract)
• granularity=day and month, month boundaries, newest-first ordering, iso_week, gap_estimated_days
• the 62-day limit and start > end

The PR claims "open-window dwell, peak, holidays, and quality" for #84, but no test checks any of them.

---

Summary: Standards 9 (2×P2, 7×P3) — worst: server-built English/locale-dependent period labels bypass i18n. Spec 6 (3×P2, 3×P3) — worst: excluded cycles not surfaced in period quality (#82 triage), plus closed days using today's multiplier until 1a3f89b lands.

🤖 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 layering is clean. SQL stays in `OccupancyRepository`, the controller is thin, contracts sit in `app/schemas/statistics.py`, and tests cover the 422 and hourly paths. No hard breach of a documented standard. ### P2: should fix before merge - **Server-built English labels bypass i18n** (judgement call). `app/services/analytics_service.py:135-144` formats labels with `strftime("%a %d %b %Y").upper()` and appends `f" ({covered} of {business} days)"`. CONTEXT.md:224 says the UI has "full dual-language support (English / Spanish) with dynamic client-side switching". Labels here are always English, and `%a`/`%b` depend on the process locale, so on the Windows prod host output can change with the service account's locale. Return structured fields (`start`, `end`, `covered_days`) and let the client format the label. - **Query count per request is high** (Duplicated Code plus performance, judgement call). `analytics_service.py:92-103` and `:172-184` repeat the same per-day loop: `facility_cycle_bounds_for_date` → `get_counted_day_flow_async` → `get_cycle_quality_async`. With `granularity=month&limit=24` that is ~730 days × 2 queries, plus every empty period back to `first_day`. Move the shared shape into one range-based "cycle summary" helper, ideally one grouped repository query. ### P3: minor - **Primitive Obsession / inconsistent typing.** `analytics_service.py:161` declares `bucket: str = "day"`, while the controller and `DailySeriesResponse` use `Literal["day", "hour"]`. Add a `Bucket = Literal[...]` alias in `schemas/statistics.py`, like `Granularity`. - **Repeated Switches.** Three module functions switch on `granularity` (`analytics_service.py:36-66`) and the label cascade at `:135-142` switches again. A small per-granularity strategy map would keep them together. - **Duplicated overlap predicate.** The gap-overlap rule is in SQL (`get_cycle_quality_async`: `start_epoch < ? AND end_epoch >= ?`) and again in Python (`analytics_service.py:228-232`); they can drift. - **Redundant loop guard.** `analytics_service.py:131`: `start <= active_day` is always true, since `start` begins at the latest closed period and only moves backwards. - **Status literal.** `statistics_controller.py:34` uses `status_code=422`; docs/standards/code-standards.md §3.1 example uses `status.HTTP_*` constants. - **Config default repeated.** `cfg.get("daily_reset_time", "04:00")` duplicated at `analytics_service.py:123` and `:164` (possible Data Clumps: `cfg`/`reset` travel together). - **Mysterious Name.** `get_counted_day_flow_async` returns an unnamed `(int, int, bool)` tuple and `get_cycle_quality_async` returns `(bool, int | None, int | None)`. A `NamedTuple` each would make call sites readable. ### No findings `docs/api/README.md`, `app/main.py`, and the PR description (follows git-and-workflow.md §2 template). ## Spec Overall this matches the triage decisions: separate `/api/statistics/` router, `require_auth`, closed periods only, the counted-camera rule, `bucket=hour` instead of a new route, no `compare=`, and the 7-day and 62-day limits. Quarantined events live in a separate table, so counted-camera queries already exclude them. ### (a) Missing or partial - **P2**: #82 triage: *"Bad-data periods (gap estimates, low Data Trust, **excluded cycles**) are included with their quality fields so the picker can mark them."* `PeriodQuality` has no field for excluded cycles, and `get_cycle_quality_async` (`app/db/occupancy_repository.py:1239`) never reads `is_trusted` or `trust_status = 'AUTO_EXCLUDED'`. The picker cannot mark an excluded cycle. - **P3**: ADR 0006: *"analytics responses expose [`occupancy_definition_cutover_at`] for chart annotation."* The daily series does not expose it, so a 62-day range crossing the cutover gets no annotation. ### (b) Scope creep Nothing significant. `min_cycle_completeness` goes beyond the sketch but follows from *"Data Trust **and** Cycle Completeness for the cycles involved"*. `end` is exclusive, as in the sketch. ### (c) Implemented but looks wrong - **P2**: `app/services/analytics_service.py:170`. Closed historical days use today's `active_exit_multiplier` for peak and dwell. Master's timeseries path uses the cycle's logged `computed_exit_multiplier`, so the same day can show different peak/dwell on the two surfaces — against ADR 0006's point that past calibration is not reconstructible. Commit 1a3f89b fixes it in stacked PR #92. Pull that fix into this PR, or state in the description that #89 is not correct without it. - **P3**: `app/schemas/statistics.py:15`. The sketch shows `"min_data_trust": 0.91`, but the code returns an int 0–100. Sound choice, not documented in `docs/api/README.md`. - **P3**: `analytics_service.py:231`. `item.end_epoch >= hour_start` marks the next hour as gap-estimated when a gap ends exactly on the hour boundary. ### Open design points | Point | Verdict | |---|---| | #87 as `bucket=hour` vs its own route | decided-and-sound | | Access (`require_auth`, separate router) | decided-and-sound | | `partial` = `covered_days < business_days`, label "N of M days" | decided-and-sound | | Week label + `iso_week` field | decided-and-sound | | Range limits → `VALIDATION_ERROR` | decided-and-sound (`:168`) | | Excluded cycles in period quality | **not addressed** | | Multiplier for closed cycles | decided-but-questionable (see c) | | Data Trust scale | decided-but-questionable (not documented) | ### Test coverage (`tests/test_statistics_periods.py`) Covered: 401 without login, excluded camera ignored, partial week with label, open-cycle rejection with `error_code`, gap marking on hourly cells, 7-day hourly limit. **P2**, not tested: - peak and its time, open-window dwell, holiday flag, `data_trust` / `cycle_completeness` values (the #84 row contract) - `granularity=day` and `month`, month boundaries, newest-first ordering, `iso_week`, `gap_estimated_days` - the 62-day limit and `start > end` The PR claims "open-window dwell, peak, holidays, and quality" for #84, but no test checks any of them. --- **Summary:** Standards 9 (2×P2, 7×P3) — worst: server-built English/locale-dependent period labels bypass i18n. Spec 6 (3×P2, 3×P3) — worst: excluded cycles not surfaced in period quality (#82 triage), plus closed days using today's multiplier until 1a3f89b lands. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg referenced this pull request 2026-09-25 08:39:18 +00:00

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

gabogg added 1 commit 2026-09-25 08:51:13 +00:00

fix: surface statistics period quality and historical calibration 1f1c688b7d

gabogg added 1 commit 2026-09-25 09:28:54 +00:00

perf: summarize statistics period quality in one query 01348e85a2

gabogg added 1 commit 2026-09-25 09:53:32 +00:00

test: cover statistics picker and daily quality contracts e4e4cff44b

gabogg commented 2026-09-25 10:28:19 +00:00

Author

Owner

Copy link

Code review — pass 2 (two-axis)

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

Pass-1 findings

Finding	Verdict	Evidence
P2: server-built English labels bypass i18n	✅ fixed	1f1c688 drops label from SelectablePeriod (app/schemas/statistics.py) and removes the strftime cascade; docs/api/README.md:129 documents structured fields for client formatting.
P2: query count per request	◐ partial	01348e8: period picker does one get_counted_cycle_quality_range_async read per period (occupancy_repository.py:1238, analytics_service.py:99). The daily-series loop (analytics_service.py:162-247) still runs ~7–9 queries per day and now adds get_calibration_log_for_cycle_async — ~500 queries for a 62-day range.
P3: bucket: str	✅ fixed	Bucket alias schemas/statistics.py:9, used in controller and analytics_service.py:153.
P3: repeated switches on granularity	◐ partial	Label cascade gone; the three module functions (analytics_service.py:37-67) still switch separately.
P3: duplicated overlap predicate	↺ regressed	Now three copies, and they disagree — see first new finding.
P3: redundant loop guard	✅ fixed	analytics_service.py:134
P3: status_code=422 literal	✅ fixed	statistics_controller.py:35 uses status.HTTP_422_UNPROCESSABLE_ENTITY
P3: daily_reset_time default repeated	❌ not addressed	analytics_service.py:126, :156. occupancy_manager.get_daily_reset_time_async() already exists.
P3: unnamed tuple returns	❌ not addressed	get_counted_day_flow_async / get_cycle_quality_async unchanged; new method returns untyped list[dict[str, Any]].

New findings

• P2: period picker and daily series disagree on gap and audit rules (Duplicated Code that has drifted). get_counted_cycle_quality_range_async (occupancy_repository.py:1268-1269) and the hourly Python check (analytics_service.py:232) now use g.end_epoch > start (the pass-1 hour-boundary fix). get_cycle_quality_async (occupancy_repository.py:~1289), still called by the daily series at analytics_service.py:169, still uses end_epoch >= ? and lacks the superseded_by IS NULL filter the range query adds. A gap ending exactly on a cycle boundary shows gap_estimated: true in /daily while /periods counts 0 gap_estimated_days. Fix: have the daily series use the range query (one call for the whole range also clears the remaining perf P2), then delete get_cycle_quality_async.
• P3: Primitive Obsession on the new repository return. get_counted_cycle_quality_range_async returns list[dict[str, Any]] read by string key (row["has_gap"], row["is_excluded"] or 0). The old method int-cast values; the new path relies on SQLite typing. NamedTuple/TypedDict.
• P3: Mysterious Name / Data Clumps. cycles: list[tuple[str, float, float]] is an anonymous (date, start, next_start) triple whose order the SQL relies on; params.extend((cycles[0][0], cycles[-1][0])) also relies on the caller passing it sorted.
• P3: multiplier = float(cfg.get("active_exit_multiplier", 1.0)) moved inside the per-day loop (analytics_service.py:172), re-reading the same config each day. Hoist it out as the fallback.
• Checked, no issue: ruff check/format pass on app tests; tests/test_statistics_periods.py passed in a throwaway worktree. Dynamic VALUES placeholders stay well under SQLite's limit (31-day month = 95 params); SQL stays in the repository.

Merge readiness (this axis): Not ready. One P2 blocks: gap/audit predicates diverge between /periods and /daily. Moving the daily series onto the range query fixes it and closes the remaining pass-1 perf P2. P3s → follow-up issue.

Spec

Head e4e4cff (fix commits 1f1c688, 01348e8, e4e4cff).

Pass-1 findings

Finding	Verdict	Evidence
P2 Excluded cycles not shown in period quality (#82 triage)	◐ partial	1f1c688/01348e8 add excluded_days (app/schemas/statistics.py:16, analytics_service.py:104), but the SQL only matches trust_status = 'AUTO_EXCLUDED' (occupancy_repository.py:1271), so manually excluded cycles are missed. See N1.
P3 No occupancy_definition_cutover_at on the daily series (ADR 0006)	✅ fixed	statistics.py:56, analytics_service.py:259; reads the REAL config column like other surfaces. No test.
P2 Closed days used today's multiplier	✅ fixed	analytics_service.py:172-181 uses the cycle's trusted computed_exit_multiplier, matching master's timeseries path (is_trusted + non-null multiplier), including the fallback for excluded days.
P3 Data Trust scale undocumented	✅ fixed	docs/api/README.md:129 ("0–100 scale").
P3 Gap ending exactly on the hour marked the next hour	✅ fixed	analytics_service.py:232 uses >.
P2 Tests didn't check the #84 row contract, day/month, limits	◐ partial	e4e4cff checks peak + time, is_holiday, trust, completeness, iso_week, exclusive end, day newest-first order, month bounds, 62-day limit, start > end. Still untested: gap_estimated_days, a true holiday, the cutover field, and the historical-multiplier path (test log has is_trusted=0, and with only IN events peak doesn't depend on the multiplier). Dwell check is only is not None.

New findings

• N1 (P2) app/db/occupancy_repository.py:1271: a.trust_status = 'AUTO_EXCLUDED' AS is_excluded. #82 triage: "Bad-data periods (gap estimates, low Data Trust, excluded cycles) are included with their quality fields so the picker can mark them." An operator's trust toggle sets MANUAL_EXCLUDED (occupancy_repository.py:1719), and ANOMALOUS_QUARANTINE also exists (occupancy_models.py:55); both yield excluded_days = 0. Confirmed by switching the test fixture to MANUAL_EXCLUDED: the test fails. Use a.is_trusted = 0, or match every *_EXCLUDED/quarantine status, and add a manual-exclusion test case.
• N2 (P3) Gap-overlap rule differs between routes: picker g.end_epoch > p.start_epoch (new range query) and hourly cells >, but the daily row's gap_estimated still uses end_epoch >= ? in get_cycle_quality_async. A gap ending exactly at 04:00 flags the daily row but not the picker's gap_estimated_days. Use one predicate. (Standards axis rates the same divergence P2.)
• N3 (P3) Daily rows have no per-day excluded flag; the picker can say "1 excluded day" but the deck can't tell which. Not required by #84; follow-up.

Open design points

Point	Verdict
bucket=hour instead of own route, require_auth, separate router, range limits	decided-and-sound
Labels removed; client formats from structured fields	decided-and-sound (i18n). Changes the #82 sketch's label; documented in the description and docs/api/README.md:129.
Excluded cycles in picker quality	decided-but-questionable (N1)
Multiplier for closed cycles	decided-and-sound (matches master)
Data Trust scale 0–100 int	decided-and-sound (now documented)

tests/test_statistics_periods.py passes at head (throwaway worktree).

Merge readiness (this axis): Not ready. N1 (P2) must be fixed. N2, N3 and remaining test gaps → follow-up issue.

---

Summary: Blocking (P2): Standards — gap/audit predicates diverge between /periods (range query) and /daily (get_cycle_quality_async); Spec — excluded_days only counts AUTO_EXCLUDED, missing MANUAL_EXCLUDED/quarantine. Rest P3. Worst per axis: Standards → predicate drift; Spec → manual exclusions invisible to the picker.

🤖 Generated with Claude Code

# Code review — pass 2 (two-axis) Re-reviewed at head `e4e4cff`. 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 ### Pass-1 findings | Finding | Verdict | Evidence | |---|---|---| | P2: server-built English labels bypass i18n | ✅ fixed | 1f1c688 drops `label` from `SelectablePeriod` (`app/schemas/statistics.py`) and removes the strftime cascade; `docs/api/README.md:129` documents structured fields for client formatting. | | P2: query count per request | ◐ partial | 01348e8: period picker does one `get_counted_cycle_quality_range_async` read per period (`occupancy_repository.py:1238`, `analytics_service.py:99`). The daily-series loop (`analytics_service.py:162-247`) still runs ~7–9 queries per day and now adds `get_calibration_log_for_cycle_async` — ~500 queries for a 62-day range. | | P3: `bucket: str` | ✅ fixed | `Bucket` alias `schemas/statistics.py:9`, used in controller and `analytics_service.py:153`. | | P3: repeated switches on granularity | ◐ partial | Label cascade gone; the three module functions (`analytics_service.py:37-67`) still switch separately. | | P3: duplicated overlap predicate | ↺ regressed | Now three copies, and they disagree — see first new finding. | | P3: redundant loop guard | ✅ fixed | `analytics_service.py:134` | | P3: `status_code=422` literal | ✅ fixed | `statistics_controller.py:35` uses `status.HTTP_422_UNPROCESSABLE_ENTITY` | | P3: `daily_reset_time` default repeated | ❌ not addressed | `analytics_service.py:126`, `:156`. `occupancy_manager.get_daily_reset_time_async()` already exists. | | P3: unnamed tuple returns | ❌ not addressed | `get_counted_day_flow_async` / `get_cycle_quality_async` unchanged; new method returns untyped `list[dict[str, Any]]`. | ### New findings - **P2: period picker and daily series disagree on gap and audit rules** (Duplicated Code that has drifted). `get_counted_cycle_quality_range_async` (`occupancy_repository.py:1268-1269`) and the hourly Python check (`analytics_service.py:232`) now use `g.end_epoch > start` (the pass-1 hour-boundary fix). `get_cycle_quality_async` (`occupancy_repository.py:~1289`), still called by the daily series at `analytics_service.py:169`, still uses `end_epoch >= ?` and lacks the `superseded_by IS NULL` filter the range query adds. A gap ending exactly on a cycle boundary shows `gap_estimated: true` in `/daily` while `/periods` counts 0 `gap_estimated_days`. Fix: have the daily series use the range query (one call for the whole range also clears the remaining perf P2), then delete `get_cycle_quality_async`. - **P3: Primitive Obsession on the new repository return.** `get_counted_cycle_quality_range_async` returns `list[dict[str, Any]]` read by string key (`row["has_gap"]`, `row["is_excluded"] or 0`). The old method int-cast values; the new path relies on SQLite typing. `NamedTuple`/TypedDict. - **P3: Mysterious Name / Data Clumps.** `cycles: list[tuple[str, float, float]]` is an anonymous (date, start, next_start) triple whose order the SQL relies on; `params.extend((cycles[0][0], cycles[-1][0]))` also relies on the caller passing it sorted. - **P3:** `multiplier = float(cfg.get("active_exit_multiplier", 1.0))` moved inside the per-day loop (`analytics_service.py:172`), re-reading the same config each day. Hoist it out as the fallback. - **Checked, no issue:** ruff check/format pass on `app tests`; `tests/test_statistics_periods.py` passed in a throwaway worktree. Dynamic `VALUES` placeholders stay well under SQLite's limit (31-day month = 95 params); SQL stays in the repository. **Merge readiness (this axis):** Not ready. One P2 blocks: gap/audit predicates diverge between `/periods` and `/daily`. Moving the daily series onto the range query fixes it and closes the remaining pass-1 perf P2. P3s → follow-up issue. ## Spec Head e4e4cff (fix commits 1f1c688, 01348e8, e4e4cff). ### Pass-1 findings | Finding | Verdict | Evidence | |---|---|---| | P2 Excluded cycles not shown in period quality (#82 triage) | ◐ partial | 1f1c688/01348e8 add `excluded_days` (`app/schemas/statistics.py:16`, `analytics_service.py:104`), but the SQL only matches `trust_status = 'AUTO_EXCLUDED'` (`occupancy_repository.py:1271`), so manually excluded cycles are missed. See N1. | | P3 No `occupancy_definition_cutover_at` on the daily series (ADR 0006) | ✅ fixed | `statistics.py:56`, `analytics_service.py:259`; reads the REAL config column like other surfaces. No test. | | P2 Closed days used today's multiplier | ✅ fixed | `analytics_service.py:172-181` uses the cycle's trusted `computed_exit_multiplier`, matching master's timeseries path (`is_trusted` + non-null multiplier), including the fallback for excluded days. | | P3 Data Trust scale undocumented | ✅ fixed | `docs/api/README.md:129` ("0–100 scale"). | | P3 Gap ending exactly on the hour marked the next hour | ✅ fixed | `analytics_service.py:232` uses `>`. | | P2 Tests didn't check the #84 row contract, day/month, limits | ◐ partial | e4e4cff checks peak + time, `is_holiday`, trust, completeness, `iso_week`, exclusive `end`, day newest-first order, month bounds, 62-day limit, `start > end`. Still untested: `gap_estimated_days`, a true holiday, the cutover field, and the historical-multiplier path (test log has `is_trusted=0`, and with only IN events peak doesn't depend on the multiplier). Dwell check is only `is not None`. | ### New findings - **N1 (P2)** `app/db/occupancy_repository.py:1271`: `a.trust_status = 'AUTO_EXCLUDED' AS is_excluded`. #82 triage: *"Bad-data periods (gap estimates, low Data Trust, **excluded cycles**) are included with their quality fields so the picker can mark them."* An operator's trust toggle sets `MANUAL_EXCLUDED` (`occupancy_repository.py:1719`), and `ANOMALOUS_QUARANTINE` also exists (`occupancy_models.py:55`); both yield `excluded_days = 0`. Confirmed by switching the test fixture to `MANUAL_EXCLUDED`: the test fails. Use `a.is_trusted = 0`, or match every `*_EXCLUDED`/quarantine status, and add a manual-exclusion test case. - **N2 (P3)** Gap-overlap rule differs between routes: picker `g.end_epoch > p.start_epoch` (new range query) and hourly cells `>`, but the daily row's `gap_estimated` still uses `end_epoch >= ?` in `get_cycle_quality_async`. A gap ending exactly at 04:00 flags the daily row but not the picker's `gap_estimated_days`. Use one predicate. (Standards axis rates the same divergence P2.) - **N3 (P3)** Daily rows have no per-day excluded flag; the picker can say "1 excluded day" but the deck can't tell which. Not required by #84; follow-up. ### Open design points | Point | Verdict | |---|---| | `bucket=hour` instead of own route, `require_auth`, separate router, range limits | decided-and-sound | | Labels removed; client formats from structured fields | decided-and-sound (i18n). Changes the #82 sketch's `label`; documented in the description and `docs/api/README.md:129`. | | Excluded cycles in picker quality | decided-but-questionable (N1) | | Multiplier for closed cycles | decided-and-sound (matches master) | | Data Trust scale 0–100 int | decided-and-sound (now documented) | `tests/test_statistics_periods.py` passes at head (throwaway worktree). **Merge readiness (this axis):** Not ready. N1 (P2) must be fixed. N2, N3 and remaining test gaps → follow-up issue. --- **Summary:** **Blocking (P2):** Standards — gap/audit predicates diverge between `/periods` (range query) and `/daily` (`get_cycle_quality_async`); Spec — `excluded_days` only counts `AUTO_EXCLUDED`, missing `MANUAL_EXCLUDED`/quarantine. Rest P3. Worst per axis: Standards → predicate drift; Spec → manual exclusions invisible to the picker. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg referenced this pull request 2026-09-25 10:28:21 +00:00

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

gabogg added 1 commit 2026-09-25 12:13:33 +00:00

fix(statistics): share one quality rule between periods and daily series ... ca30e17160

Count manually excluded and quarantined cycles in `excluded_days`, not
only automatic exclusions. The daily series now reads gap and audit
quality through the same range query as the period picker, so a gap
ending exactly at the reset no longer flags the next day on one route
and not the other. Removes the diverged per-day quality query and the
per-day config re-read.

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

gabogg referenced this pull request 2026-09-25 12:14:43 +00:00

follow-up(statistics): P3 cleanups from #89 review (periods and daily series) #96

gabogg merged commit 3541af5638 into master 2026-09-25 12:17:17 +00:00

gabogg deleted branch feat/statistics-periods-daily 2026-09-25 12:17:17 +00:00

gabogg referenced this pull request from a commit 2026-09-25 12:17:18 +00:00

feat(statistics): closed periods and daily series (#82, #84) (#89)

gabogg referenced this pull request 2026-09-25 17:09:00 +00:00

feat(analytics): list selectable statistics periods #82

gabogg referenced this pull request 2026-09-25 17:09:02 +00:00

feat(analytics): daily totals series for a date range #84

gabogg referenced this pull request 2026-09-26 10:25:45 +00:00

refactor(statistics): resolve review follow-ups across statistics domain #132

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

Subscribe

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!89

No description provided.