refactor(statistics): baseline and query follow-ups #167

Merged

gabogg merged 9 commits from refactor/statistics-baseline-followups into master 2026-10-02 23:51:22 +00:00

Conversation 5 Commits 9 Files changed 10 +1232 -246

gabogg commented 2026-09-28 00:11:46 +00:00

Owner

Copy link

Summary

Batch the daily statistics series' counted flow, calibration logs, schedule context, and open-window counts with unnested joins for indexed per-cycle evaluation. Reuse configuration in baseline sample reads, guard hourly baselines against differing bucket labels while averaging duplicate clock labels positionally by index, validate baseline bucket size before aggregation, type repository batch inputs with CycleWindow and typed returns with RangeCounts and CalibrationLogRecord, narrow candidate sample aggregation for hourly and dayparts baselines, and cover edge cases (verdict priority, sparse baselines, non-trivial shares, 62-day series query count). Multi-day Riemann step calculation batching is deferred to linked issue #203. The issue-by-issue evidence and remaining work are in docs/audit/statistics-baseline-followups-pr-plan.md.

Architectural impact

The occupancy repository now offers batched read contracts for cycle flow, calibration logs, and named windows accepting Sequence[CycleWindow] and returning typed models (CountedDayFlow, RangeCounts, CalibrationLogRecord). The analytics service combines those reads, narrows baseline candidate evaluation to direct bucket flow (_get_sample_hourly_in_counts_async) and dayparts (_compute_dayparts_for_day_async) bypassing full responses and redundant cycle quality queries, and preserves existing response shapes, Closed Day rules, and ADR 0006 multiplier behavior. No schema migration or new dependency is included.

Verification

• rtk pytest: 526 passed.
• rtk ruff check .: passed.
• rtk ruff format --check .: passed.
• python3 scripts/check_docs.py: passed.
• Pre-commit hooks, including pytest gate: passed.

Checklist

• Implement and test the batch daily-series inputs and baseline edge cases.
• Map #96, #97, and #124 criteria to existing or changed code and tests in the PR plan.
• Complete #124's config and candidate-trust follow-ups.
• Unify batch repository contracts using CycleWindow, RangeCounts, and CalibrationLogRecord.
• Multi-day Riemann batching documented and deferred to linked issue #203.
• Finish #97's narrower per-day baseline sample aggregation, bucket-label guard, and positional averaging.
• Complete the required three review passes and their findings before merge.

This PR addresses #96 and #97 and closes #124 when merged. Keep the ready-for-agent role while implementation remains.

Review protocol

This PR addresses review follow-up issues, so it follows the three-pass path in docs/standards/git-and-workflow.md. First and second passes address every P1/P2/P3 finding; the third addresses P1/P2 and files linked follow-up issues for any deferred P3.

## Summary Batch the daily statistics series' counted flow, calibration logs, schedule context, and open-window counts with unnested joins for indexed per-cycle evaluation. Reuse configuration in baseline sample reads, guard hourly baselines against differing bucket labels while averaging duplicate clock labels positionally by index, validate baseline bucket size before aggregation, type repository batch inputs with `CycleWindow` and typed returns with `RangeCounts` and `CalibrationLogRecord`, narrow candidate sample aggregation for hourly and dayparts baselines, and cover edge cases (verdict priority, sparse baselines, non-trivial shares, 62-day series query count). Multi-day Riemann step calculation batching is deferred to linked issue #203. The issue-by-issue evidence and remaining work are in docs/audit/statistics-baseline-followups-pr-plan.md. ## Architectural impact The occupancy repository now offers batched read contracts for cycle flow, calibration logs, and named windows accepting `Sequence[CycleWindow]` and returning typed models (`CountedDayFlow`, `RangeCounts`, `CalibrationLogRecord`). The analytics service combines those reads, narrows baseline candidate evaluation to direct bucket flow (`_get_sample_hourly_in_counts_async`) and dayparts (`_compute_dayparts_for_day_async`) bypassing full responses and redundant cycle quality queries, and preserves existing response shapes, Closed Day rules, and ADR 0006 multiplier behavior. No schema migration or new dependency is included. ## Verification - `rtk pytest`: 526 passed. - `rtk ruff check .`: passed. - `rtk ruff format --check .`: passed. - `python3 scripts/check_docs.py`: passed. - Pre-commit hooks, including pytest gate: passed. ## Checklist - [x] Implement and test the batch daily-series inputs and baseline edge cases. - [x] Map #96, #97, and #124 criteria to existing or changed code and tests in the PR plan. - [x] Complete #124's config and candidate-trust follow-ups. - [x] Unify batch repository contracts using `CycleWindow`, `RangeCounts`, and `CalibrationLogRecord`. - [x] Multi-day Riemann batching documented and deferred to linked issue #203. - [x] Finish #97's narrower per-day baseline sample aggregation, bucket-label guard, and positional averaging. - [ ] Complete the required three review passes and their findings before merge. This PR addresses #96 and #97 and closes #124 when merged. Keep the ready-for-agent role while implementation remains. ## Review protocol This PR addresses review follow-up issues, so it follows the three-pass path in docs/standards/git-and-workflow.md. First and second passes address every P1/P2/P3 finding; the third addresses P1/P2 and files linked follow-up issues for any deferred P3.

gabogg added 1 commit 2026-09-28 00:11:47 +00:00

docs(statistics): map baseline follow-up work 4b5ba875bb

gabogg added the

ready-for-agent

label 2026-09-28 00:25:03 +00:00

gabogg added 1 commit 2026-09-28 00:34:29 +00:00

Merge remote-tracking branch 'origin/master' into refactor/statistics-baseline-followups a865f2d585

gabogg added 1 commit 2026-09-30 14:51:34 +00:00

refactor(statistics): batch daily series inputs and cover baseline edge cases cc1eea24b4

gabogg changed title from WIP: refactor(statistics): baseline and query follow-ups to refactor(statistics): baseline and query follow-ups 2026-09-30 14:52:48 +00:00

gabogg added 1 commit 2026-09-30 15:01:39 +00:00

Merge remote-tracking branch 'origin/master' into refactor/statistics-baseline-followups ... 51623da40c

# Conflicts:
#	app/controllers/statistics_controller.py
#	app/facility_time.py

gabogg added 1 commit 2026-10-02 11:46:47 +00:00

refactor(statistics): narrow baseline sample aggregation and type secondary audit inputs (#96, #97) 3cb0db0ab8

gabogg added 1 commit 2026-10-02 12:02:23 +00:00

refactor(statistics): bound range event filter and share bucket time label (#96, #97) 8a28366069

gabogg added 1 commit 2026-10-02 14:04:59 +00:00

Merge remote-tracking branch 'origin/master' into refactor/statistics-baseline-followups c42844c4d8

gabogg reviewed 2026-10-02 14:40:26 +00:00

gabogg left a comment

Copy link

Code review, pass 1 (origin/master...c42844c, spec #96, #97, #124)

Result: 1 P1, 2 P2s and 13 P3s. This PR addresses follow-up issues, so it takes the three-pass path: every finding on passes 1 and 2 gets fixed.

Verification:

• ruff check and format: clean.
• Targeted tests: 23 + 33 passed.
• EXPLAIN QUERY PLAN and timing probe: SQLite 3.51, 200k events, the repo's real indexes.

Standards

P1

1. Performance regression: the new batched flow query is about 150x slower (occupancy_repository.py:1644 get_counted_cycle_flow_range_async).
   • The LEFT JOIN (people_counting_events e … ) ON e.timestamp_epoch >= p.start_epoch … has no outer time bound. SQLite materializes the whole events table and scans it once per cycle (MATERIALIZE (join-3), then SCAN … LEFT-JOIN).
   • 62 cycles: 1.51 s, against 0.009 s for the old per-day queries. On prod it grows as all events × cycles.
   • It hits the daily series and the Usual Weekday Baseline.
   • get_counts_in_ranges_async (:1712) already bounds min_start/max_end inside the join (0.005 s, indexed). Apply the same bound here.
   • test_daily_series_62_days_query_count counts connections, not query cost, so it can't catch this. Add a plan or bound assertion.

P2
2. Duplicated Code against ADR 0006 (analytics_service.py:973-982). The daily series copies the trust rule from _trusted_cycle_log_async (:423), so one calibration-honesty rule now has two copies. Extract a pure _trusted_multiplier(log, default) and use it in both.
3. Duplicated Code (judgement call, analytics_service.py:948-963).

• The open/close window, including the overnight close_epoch <= open_epoch rollover, is a hand copy of OccupancyService._schedule_epochs.
• Its behaviour is verified equivalent.
• Share it on ScheduleCalendar or in facility_time.

P3
4. DST fall-back (analytics_service.py:1284). dict(zip(labels, counts)) keeps only the last bucket when two share a clock label, where the old code appended both. Matches Spec #97's missing bucket-label guard.
5. _get_sample_hourly_in_counts_async (1254-1284) repeats the main path's bucket loop. Have the main path use it.
6. Speculative Generality. The new reset and reset_time_str params on get_dwell_dayparts_async/get_hourly_timeseries_async (1148, 1294) have no callers, and they let a reset disagree with cfg. Delete them.
7. _daily_reset_time = configured_reset_time (:74) is a bare alias. Also, OccupancyService.get_daily_reset_time_async still uses .get(..., "04:00"), so the defaults diverge.
8. validate_closed_statistics_day_async (:379) now also returns config; consider closed_day_config_async.
9. TimeRangeTarget (occupancy_models.py:46) has the same shape as CycleQualityTarget, and the flow and log methods take a "quality" type. Use one CycleWindow type for both.
10. Redundant guard (:1001): not is_closed and day_str in open_window_by_day.

Checked and equivalent:

• calibration-log windowing: the change is 1 ms, and the id DESC tiebreak is an improvement;
• the counts-in-ranges bounds;
• the peak skip;
• the earlier bucket guard;
• the holiday-hours .get;
• DwellDay serialization;
• layering;
• the tests (real sqlite).

Spec

Checklist:

• #124: all items done. Most were already done on master; this PR adds the summary flow re-read. "Closes #124" is justified.
• #96:
  • partial: the daily-series query count, and typed returns for the new methods;
  • moved to #106: the per-day excluded flag;
  • everything else is done.
• #97:
  • partial: active_business_day;
  • missing: the bucket-label guard;
  • moved to #105/#106: changed opening hours;
  • everything else is done.

P3
11. #97, missing: "Hourly baseline has no guard for differing bucket labels across source days." It is neither addressed nor deferred (see #4).
12. #97, partial: "extract active_business_day(reset)". analytics_service.py:886 still uses facility_cycle_bounds(span[0], reset), yet the plan doc marks the item Complete.
13. #96, partial: "A 62-day range still costs a few hundred queries." Peak and average are still read per day. The deferral is in the plan doc but has no linked follow-up issue.
14. #96, partial: "Use a NamedTuple/TypedDict."
- get_counts_in_ranges_async returns dict[str, tuple[int, int]].
- get_calibration_logs_for_cycles_async returns dict[str, dict[str, Any]].
- Both take bare tuple[str, float, float] as well.
15. The plan doc is wrong in places.
- It credits a handle_controller_errors() helper that doesn't exist; the helper is domain_errors().
- Lines 30, 33, 35, 36 and 51-53 present items already done on master as this PR's work.
- It omits the #97 opening-hours item that moved to #105/#106.
16. Trust predicate duplicated (same as Standards P2-2).

Scope creep: small and justified: the holiday-hours fallback, the id DESC tiebreak, the skipped config reads, and DwellDay typing.

Summary

• Standards: 1 P1, 2 P2s and 7 P3s. The worst is the unbounded join in the batched flow query, about 150x slower.
• Spec: no P1s or P2s, and 6 P3s. The worst is the missing bucket-label guard.

🤖 Generated with Claude Code

## Code review, pass 1 (`origin/master...c42844c`, spec #96, #97, #124) Result: **1 P1, 2 P2s and 13 P3s.** This PR addresses follow-up issues, so it takes the three-pass path: every finding on passes 1 and 2 gets fixed. Verification: - ruff check and format: clean. - Targeted tests: 23 + 33 passed. - EXPLAIN QUERY PLAN and timing probe: SQLite 3.51, 200k events, the repo's real indexes. ## Standards **P1** 1. **Performance regression: the new batched flow query is about 150x slower** (`occupancy_repository.py:1644` `get_counted_cycle_flow_range_async`). - The `LEFT JOIN (people_counting_events e … ) ON e.timestamp_epoch >= p.start_epoch …` has no outer time bound. SQLite materializes the whole events table and scans it once per cycle (`MATERIALIZE (join-3)`, then `SCAN … LEFT-JOIN`). - 62 cycles: 1.51 s, against 0.009 s for the old per-day queries. On prod it grows as all events × cycles. - It hits the daily series and the Usual Weekday Baseline. - `get_counts_in_ranges_async` (:1712) already bounds `min_start`/`max_end` inside the join (0.005 s, indexed). Apply the same bound here. - `test_daily_series_62_days_query_count` counts connections, not query cost, so it can't catch this. Add a plan or bound assertion. **P2** 2. **Duplicated Code against ADR 0006** (`analytics_service.py:973-982`). The daily series copies the trust rule from `_trusted_cycle_log_async` (:423), so one calibration-honesty rule now has two copies. Extract a pure `_trusted_multiplier(log, default)` and use it in both. 3. **Duplicated Code** (judgement call, `analytics_service.py:948-963`). - The open/close window, including the overnight `close_epoch <= open_epoch` rollover, is a hand copy of `OccupancyService._schedule_epochs`. - Its behaviour is verified equivalent. - Share it on `ScheduleCalendar` or in facility_time. **P3** 4. **DST fall-back** (`analytics_service.py:1284`). `dict(zip(labels, counts))` keeps only the last bucket when two share a clock label, where the old code appended both. Matches Spec #97's missing bucket-label guard. 5. **`_get_sample_hourly_in_counts_async` (1254-1284)** repeats the main path's bucket loop. Have the main path use it. 6. **Speculative Generality.** The new `reset` and `reset_time_str` params on `get_dwell_dayparts_async`/`get_hourly_timeseries_async` (1148, 1294) have no callers, and they let a reset disagree with `cfg`. Delete them. 7. **`_daily_reset_time = configured_reset_time` (:74)** is a bare alias. Also, `OccupancyService.get_daily_reset_time_async` still uses `.get(..., "04:00")`, so the defaults diverge. 8. **`validate_closed_statistics_day_async` (:379)** now also returns config; consider `closed_day_config_async`. 9. **`TimeRangeTarget` (`occupancy_models.py:46`)** has the same shape as `CycleQualityTarget`, and the flow and log methods take a "quality" type. Use one `CycleWindow` type for both. 10. **Redundant guard (:1001):** `not is_closed and day_str in open_window_by_day`. **Checked and equivalent:** - calibration-log windowing: the change is 1 ms, and the `id DESC` tiebreak is an improvement; - the counts-in-ranges bounds; - the peak skip; - the earlier bucket guard; - the holiday-hours `.get`; - `DwellDay` serialization; - layering; - the tests (real sqlite). ## Spec **Checklist:** - **#124:** all items done. Most were already done on master; this PR adds the summary flow re-read. "Closes #124" is justified. - **#96:** - partial: the daily-series query count, and typed returns for the new methods; - moved to #106: the per-day excluded flag; - everything else is done. - **#97:** - partial: `active_business_day`; - missing: the bucket-label guard; - moved to #105/#106: changed opening hours; - everything else is done. **P3** 11. **#97, missing:** *"Hourly baseline has no guard for differing bucket labels across source days."* It is neither addressed nor deferred (see #4). 12. **#97, partial:** *"extract active_business_day(reset)"*. `analytics_service.py:886` still uses `facility_cycle_bounds(span[0], reset)`, yet the plan doc marks the item Complete. 13. **#96, partial:** *"A 62-day range still costs a few hundred queries."* Peak and average are still read per day. The deferral is in the plan doc but has no linked follow-up issue. 14. **#96, partial:** *"Use a NamedTuple/TypedDict."* - `get_counts_in_ranges_async` returns `dict[str, tuple[int, int]]`. - `get_calibration_logs_for_cycles_async` returns `dict[str, dict[str, Any]]`. - Both take bare `tuple[str, float, float]` as well. 15. **The plan doc is wrong in places.** - It credits a `handle_controller_errors()` helper that doesn't exist; the helper is `domain_errors()`. - Lines 30, 33, 35, 36 and 51-53 present items already done on master as this PR's work. - It omits the #97 opening-hours item that moved to #105/#106. 16. **Trust predicate duplicated** (same as Standards P2-2). **Scope creep:** small and justified: the holiday-hours fallback, the `id DESC` tiebreak, the skipped config reads, and `DwellDay` typing. ## Summary - **Standards:** 1 P1, 2 P2s and 7 P3s. The worst is the unbounded join in the batched flow query, about 150x slower. - **Spec:** no P1s or P2s, and 6 P3s. The worst is the missing bucket-label guard. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg referenced this pull request 2026-10-02 15:00:48 +00:00

perf(statistics): batch per-cycle peak and average occupancy Riemann integration #203

gabogg referenced this pull request from a commit 2026-10-02 15:10:25 +00:00

fix(statistics): address first pass review findings for PR #167

gabogg added 1 commit 2026-10-02 15:10:25 +00:00

fix(statistics): address first pass review findings for PR #167 ... eda7191701

- Bound people_counting_events join in get_counted_cycle_flow_range_async with min_start/max_end and verify index range scan in test
- Extract _is_trusted_log and _trusted_multiplier helpers in analytics_service to eliminate duplicate multiplier evaluation logic
- Extract schedule_epochs helper in facility_time and reuse in ScheduleCalendar, OccupancyService, and daily series open window loop
- Return list of (label, in_count) tuples for sample hourly flow to handle DST fall-back duplicate labels, and guard candidate bucket labels
- Extract _compute_hourly_buckets_for_day_async helper in analytics_service
- Clean up unused parameters (reset in get_dwell_dayparts_async, reset_time_str in get_hourly_timeseries_async)
- Remove _daily_reset_time alias and use configured_reset_time everywhere
- Rename validate_closed_statistics_day_async to closed_day_config_async
- Unify CycleQualityTarget and TimeRangeTarget into CycleWindow; add RangeCounts and CalibrationLogRecord typed models
- Clean up redundant closed-day check in daily series loop
- Use active_business_day in list_statistics_periods_async
- Link Riemann integration batching follow-up issue #203
- Update statistics-baseline-followups-pr-plan.md audit doc

gabogg commented 2026-10-02 15:11:21 +00:00

Author

Owner

Copy link

Pass 1 fixes (eda7191)

All 16 findings (1 P1, 2 P2s, 13 P3s) from pass 1 review r13 have been resolved:

Standards

1. P1 — Performance regression in batched flow query: Added min_start and max_end bounds to the people_counting_events subquery join in get_counted_cycle_flow_range_async (app/db/occupancy_repository.py). Updated test_daily_series_62_days_query_count with an EXPLAIN QUERY PLAN assertion verifying indexed search over whole-table scan.
2. P2 — Duplicated trust rule vs ADR 0006: Extracted _is_trusted_log(log) and pure _trusted_multiplier(log, default) in app/services/analytics_service.py. Reused in _trusted_cycle_log_async, _trusted_cycle_multiplier_async, and the daily series loop.
3. P2 — Duplicated open/close window calculation: Extracted schedule_epochs(day, open_time, close_time) in app/facility_time.py and ScheduleCalendar.schedule_epochs(day). Reused in OccupancyService._schedule_epochs and get_statistics_daily_series_async.
4. P3 — DST fall-back clock labels: Changed candidate hourly flow aggregation to return a list of (label, in_count) tuples instead of a dict, preventing key collisions when multiple buckets share a clock label on DST fall-back.
5. P3 — Duplicated bucket loop: Extracted _compute_hourly_buckets_for_day_async in app/services/analytics_service.py to build and populate flow buckets once; reused by both get_hourly_timeseries_async and _get_sample_hourly_in_counts_async.
6. P3 — Speculative parameters: Removed unused reset from get_dwell_dayparts_async and reset_time_str from get_hourly_timeseries_async. Both resolve internally via configured_reset_time(cfg).
7. P3 — Bare alias and diverging defaults: Removed _daily_reset_time alias in analytics_service.py and standardized on configured_reset_time(cfg) across all call sites, including OccupancyService.get_daily_reset_time_async.
8. P3 — Method naming: Renamed validate_closed_statistics_day_async to closed_day_config_async in analytics_service.py (preserving backward compatibility alias) and updated statistics_controller.py.
9. P3 — Unified CycleWindow type: Unified CycleQualityTarget and TimeRangeTarget into CycleWindow in app/schemas/occupancy_models.py (preserving aliases).
10. P3 — Redundant guard: Removed redundant not is_closed and in if day_str in open_window_by_day: in get_statistics_daily_series_async.

Spec

11. P3 — Missing bucket-label guard on hourly baseline (#97): Added guard if [label for label, _ in sample] != target_labels: continue with usable_dates and INSUFFICIENT_USUAL_WEEKDAYS check in get_hourly_timeseries_async. Added tests for mismatched bucket labels and DST fall-back averaging in tests/test_statistics_baselines.py.
12. P3 — Partial active_business_day extraction (#97): Updated span[0] civil date extraction in list_statistics_periods_async to first_day = active_business_day(span[0], reset).
13. P3 — Unlinked deferral of Riemann batching (#96): Opened linked follow-up issue #203 (perf(statistics): batch per-cycle peak and average occupancy Riemann integration) labeled ready-for-agent.
14. P3 — Typed repository returns (#96): Added RangeCounts(NamedTuple) returned by get_counts_in_ranges_async, and CalibrationLogRecord(TypedDict) returned by get_calibration_logs_for_cycles_async.
15. P3 — Audit plan doc inaccuracies: Corrected docs/audit/statistics-baseline-followups-pr-plan.md to credit domain_errors(), clarify items that were already merged on master prior to branch, document the opening-hours item moving to #105/#106, and link follow-up #203.
16. P3 — Trust predicate duplicated: Resolved alongside Finding 2 via _trusted_multiplier.

All 526 tests passing, ruff lint/format clean, check_docs clean. Ready for pass 2 review.

## Pass 1 fixes (eda7191) All 16 findings (1 P1, 2 P2s, 13 P3s) from pass 1 review `r13` have been resolved: ### Standards 1. **P1 — Performance regression in batched flow query:** Added `min_start` and `max_end` bounds to the `people_counting_events` subquery join in `get_counted_cycle_flow_range_async` (`app/db/occupancy_repository.py`). Updated `test_daily_series_62_days_query_count` with an `EXPLAIN QUERY PLAN` assertion verifying indexed search over whole-table scan. 2. **P2 — Duplicated trust rule vs ADR 0006:** Extracted `_is_trusted_log(log)` and pure `_trusted_multiplier(log, default)` in `app/services/analytics_service.py`. Reused in `_trusted_cycle_log_async`, `_trusted_cycle_multiplier_async`, and the daily series loop. 3. **P2 — Duplicated open/close window calculation:** Extracted `schedule_epochs(day, open_time, close_time)` in `app/facility_time.py` and `ScheduleCalendar.schedule_epochs(day)`. Reused in `OccupancyService._schedule_epochs` and `get_statistics_daily_series_async`. 4. **P3 — DST fall-back clock labels:** Changed candidate hourly flow aggregation to return a list of `(label, in_count)` tuples instead of a dict, preventing key collisions when multiple buckets share a clock label on DST fall-back. 5. **P3 — Duplicated bucket loop:** Extracted `_compute_hourly_buckets_for_day_async` in `app/services/analytics_service.py` to build and populate flow buckets once; reused by both `get_hourly_timeseries_async` and `_get_sample_hourly_in_counts_async`. 6. **P3 — Speculative parameters:** Removed unused `reset` from `get_dwell_dayparts_async` and `reset_time_str` from `get_hourly_timeseries_async`. Both resolve internally via `configured_reset_time(cfg)`. 7. **P3 — Bare alias and diverging defaults:** Removed `_daily_reset_time` alias in `analytics_service.py` and standardized on `configured_reset_time(cfg)` across all call sites, including `OccupancyService.get_daily_reset_time_async`. 8. **P3 — Method naming:** Renamed `validate_closed_statistics_day_async` to `closed_day_config_async` in `analytics_service.py` (preserving backward compatibility alias) and updated `statistics_controller.py`. 9. **P3 — Unified CycleWindow type:** Unified `CycleQualityTarget` and `TimeRangeTarget` into `CycleWindow` in `app/schemas/occupancy_models.py` (preserving aliases). 10. **P3 — Redundant guard:** Removed redundant `not is_closed and` in `if day_str in open_window_by_day:` in `get_statistics_daily_series_async`. ### Spec 11. **P3 — Missing bucket-label guard on hourly baseline (#97):** Added guard `if [label for label, _ in sample] != target_labels: continue` with `usable_dates` and `INSUFFICIENT_USUAL_WEEKDAYS` check in `get_hourly_timeseries_async`. Added tests for mismatched bucket labels and DST fall-back averaging in `tests/test_statistics_baselines.py`. 12. **P3 — Partial active_business_day extraction (#97):** Updated `span[0]` civil date extraction in `list_statistics_periods_async` to `first_day = active_business_day(span[0], reset)`. 13. **P3 — Unlinked deferral of Riemann batching (#96):** Opened linked follow-up issue #203 (`perf(statistics): batch per-cycle peak and average occupancy Riemann integration`) labeled `ready-for-agent`. 14. **P3 — Typed repository returns (#96):** Added `RangeCounts(NamedTuple)` returned by `get_counts_in_ranges_async`, and `CalibrationLogRecord(TypedDict)` returned by `get_calibration_logs_for_cycles_async`. 15. **P3 — Audit plan doc inaccuracies:** Corrected `docs/audit/statistics-baseline-followups-pr-plan.md` to credit `domain_errors()`, clarify items that were already merged on `master` prior to branch, document the opening-hours item moving to #105/#106, and link follow-up #203. 16. **P3 — Trust predicate duplicated:** Resolved alongside Finding 2 via `_trusted_multiplier`. All 526 tests passing, ruff lint/format clean, check_docs clean. Ready for pass 2 review.

gabogg referenced this pull request 2026-10-02 15:49:30 +00:00

research(docs): reinforce documentation, enforce its maintenance under agentic development, and shape a knowledge base for agents #208

gabogg reviewed 2026-10-02 16:04:07 +00:00

gabogg left a comment

Copy link

Code review, pass 2 (origin/master...eda7191, spec #96, #97, #124)

Result: 1 P1, 1 P2 and 8 P3s. This PR addresses follow-up issues, so it takes the three-pass path: fix every finding below, push, and request a third pass. Several Standards and Spec findings are the same issue: S5 with Sp1, S3 with Sp3, and the typed-argument items.

Verification:

• ruff check and format: clean.
• test_statistics_baselines.py and test_statistics_periods.py: 25 passed.
• Probe scratchpad/probe167p2.py: SQLite 3.51.2, the repo's real schema and 4 indexes, 200k events, 20 cameras (1 excluded), 62 cycles, best of 3.

Query	365-day data (32k in range)	62-day data (190k in range)
Old per-day ×62	0.14 s	0.77–0.84 s
c42844c (unbounded)	3.11 s	3.41 s
eda7191 (bounded)	0.37–0.59 s	2.93–4.21 s
Proposed fix (see P1)	0.15 s	0.96 s

Standards

Pass-1 fixes:

• Fixed: 2, 3, 4, 5, 6, 7 and 10.
• Partial: 1 (P1 below), 8 and 9 (P3-3 below).
• The controller rename, active_business_day and get_daily_reset_time_async check out.

P1

1. The batched flow query is still slower than the old per-day path (occupancy_repository.py:1645-1670).
   • The plan is still MATERIALIZE (join-63) … SCAN p | SCAN (join-63) LEFT-JOIN. The bound only shrinks the materialized set, and every cycle still scans all events in range, so cost is cycles × events in range.
   • It is 2.6–5x slower than per-day, and worst when the range covers all the data, which is the normal daily-series case.
   • Fix: drop the parenthesised join so e is searched per cycle:

     LEFT JOIN people_counting_events e
       ON e.timestamp_epoch >= p.start_epoch AND e.timestamp_epoch < p.end_epoch
      AND e.camera_index_code IN (SELECT camera_index_code FROM counting_cameras c WHERE <filter>)
     

     The plan becomes SCAN p | SEARCH e USING INDEX … (camera_index_code=? AND timestamp_epoch>? AND timestamp_epoch<?), the totals are identical, and the speed matches per-day.
   • The new plan test proves nothing (tests/test_statistics_periods.py:~552-558). "SEARCH e USING" also appears in the unbounded plan. Assert that the plan has no MATERIALIZE and no SCAN (join-.
   • get_counts_in_ranges_async (:1720) has the same shape and is also new in this PR. Pass 1 wrongly judged it fine: 62 open windows take 0.62 s and 2.73 s. Apply the same rewrite and the same plan assertion.

P2
2. CalibrationLogRecord (occupancy_models.py:62-78) doesn't match the occupancy_calibration_logs schema (database.py:620-680).

• It declares non-columns: previous_exit_multiplier, baseline_offset and patrol_guard_count. The last two are config keys.
• It omits real columns, including target_guard_count, which analytics_service.py:1369 reads.
• It types is_trusted/is_current as bool, but SQLite returns int.
• total=False plus cast hides all of this. Derive the fields from the schema.

P3
3. Speculative Generality: unused "backward compatibility" aliases.

• TimeRangeTarget = CycleWindow (occupancy_models.py:52) and validate_closed_statistics_day_async = closed_day_config_async (analytics_service.py:408) have no callers.
• CycleQualityTarget is still used at analytics_service.py:35,374,643,825,845,848 and in the tests, so one file uses two names for one type.
• Delete the aliases and finish the rename. There are no external consumers.

4. CycleWindow.cycle_date (:45-47) is an alias property for window_id. Keep one name.
5. Duplicate-label averaging (analytics_service.py:1513-1526).
   • Averaging is keyed by label, so a duplicated clock label blends both buckets: the test expects 15.0, where averaging by position gives 10.0.
   • The new guard already guarantees identical label sequences, so average by index.
   • The test pins an artefact: no real candidate shares a DST fold with the target.
6. _trusted_multiplier (:83-87) needs # type: ignore[index] only because narrowing goes through _is_trusted_log. Return the multiplier or None from one function.

Spec

Pass-1 fix claims (c3431):

• #12: active_business_day(span[0], reset). True.
• #13: #203 exists, is open and ready-for-agent, cites #96/#167, and is linked from the plan doc. True.
• #15: the plan doc is corrected (domain_errors(), "landed on master prior to branch", the #105/#106 move, #203). True.
• #16: _is_trusted_log/_trusted_multiplier used at all three sites. True.
• #11: the guard is present and correct, but the DST averaging is wrong (Sp1).
• #14: partial (Sp2).

P3

1. Duplicate clock labels are pooled instead of averaged by position (same as Standards P3-5). #97: "Hourly baseline has no guard for differing bucket labels across source days."
2. #96's typed arguments are still partial. #96 says: "The cycles: list[tuple[str, float, float]] argument is an anonymous triple." All four batch methods still accept CycleWindow | tuple[str, float, float] (occupancy_repository.py:1646, 1684, 1721, 1789). Only the returns gained RangeCounts/CalibrationLogRecord. Accept CycleWindow only.
3. The CycleWindow unification is half done (same as Standards P3-3).
4. The PR description is stale.
   • It still names TimeRangeTarget/CycleQualityTarget as the contracts and says "465 passed"; the fix reply says 526.
   • It doesn't mention CycleWindow, RangeCounts, CalibrationLogRecord, the bucket-label guard, or the #203 deferral.

Checklist:

• #96: the query count is partial, with the rest deferred to #203 (linked). Typed returns are done, but the triple argument remains. Everything else is done.
• #97: done; the bucket-label guard has the averaging caveat.
• #124: all done, so "Closes #124" is justified.

Summary

• Standards: 1 P1, 1 P2 and 4 P3s. The worst is the batched flow query, which is still 2.6–5x slower than per-day.
• Spec: 4 P3s. The worst is the duplicate-label averaging.

🤖 Generated with Claude Code

## Code review, pass 2 (`origin/master...eda7191`, spec #96, #97, #124) Result: **1 P1, 1 P2 and 8 P3s.** This PR addresses follow-up issues, so it takes the three-pass path: fix **every** finding below, push, and request a **third pass**. Several Standards and Spec findings are the same issue: S5 with Sp1, S3 with Sp3, and the typed-argument items. Verification: - ruff check and format: clean. - `test_statistics_baselines.py` and `test_statistics_periods.py`: 25 passed. - Probe `scratchpad/probe167p2.py`: SQLite 3.51.2, the repo's real schema and 4 indexes, 200k events, 20 cameras (1 excluded), 62 cycles, best of 3. | Query | 365-day data (32k in range) | 62-day data (190k in range) | |---|---|---| | Old per-day ×62 | 0.14 s | 0.77–0.84 s | | c42844c (unbounded) | 3.11 s | 3.41 s | | **eda7191 (bounded)** | **0.37–0.59 s** | **2.93–4.21 s** | | Proposed fix (see P1) | 0.15 s | 0.96 s | ## Standards **Pass-1 fixes:** - Fixed: 2, 3, 4, 5, 6, 7 and 10. - Partial: 1 (P1 below), 8 and 9 (P3-3 below). - The controller rename, `active_business_day` and `get_daily_reset_time_async` check out. **P1** 1. **The batched flow query is still slower than the old per-day path** (`occupancy_repository.py:1645-1670`). - The plan is still `MATERIALIZE (join-63) … SCAN p | SCAN (join-63) LEFT-JOIN`. The bound only shrinks the materialized set, and every cycle still scans all events in range, so cost is cycles × events in range. - It is 2.6–5x slower than per-day, and worst when the range covers all the data, which is the normal daily-series case. - **Fix:** drop the parenthesised join so `e` is searched per cycle: ```sql LEFT JOIN people_counting_events e ON e.timestamp_epoch >= p.start_epoch AND e.timestamp_epoch < p.end_epoch AND e.camera_index_code IN (SELECT camera_index_code FROM counting_cameras c WHERE <filter>) ``` The plan becomes `SCAN p | SEARCH e USING INDEX … (camera_index_code=? AND timestamp_epoch>? AND timestamp_epoch<?)`, the totals are identical, and the speed matches per-day. - **The new plan test proves nothing** (`tests/test_statistics_periods.py:~552-558`). `"SEARCH e USING"` also appears in the unbounded plan. Assert that the plan has **no `MATERIALIZE` and no `SCAN (join-`**. - **`get_counts_in_ranges_async` (:1720) has the same shape and is also new in this PR.** Pass 1 wrongly judged it fine: 62 open windows take 0.62 s and 2.73 s. Apply the same rewrite and the same plan assertion. **P2** 2. **`CalibrationLogRecord` (`occupancy_models.py:62-78`) doesn't match the `occupancy_calibration_logs` schema** (`database.py:620-680`). - It declares non-columns: `previous_exit_multiplier`, `baseline_offset` and `patrol_guard_count`. The last two are config keys. - It omits real columns, including `target_guard_count`, which `analytics_service.py:1369` reads. - It types `is_trusted`/`is_current` as `bool`, but SQLite returns int. - `total=False` plus `cast` hides all of this. Derive the fields from the schema. **P3** 3. **Speculative Generality: unused "backward compatibility" aliases.** - `TimeRangeTarget = CycleWindow` (`occupancy_models.py:52`) and `validate_closed_statistics_day_async = closed_day_config_async` (`analytics_service.py:408`) have no callers. - `CycleQualityTarget` is still used at `analytics_service.py:35,374,643,825,845,848` and in the tests, so one file uses two names for one type. - Delete the aliases and finish the rename. There are no external consumers. 4. **`CycleWindow.cycle_date` (:45-47)** is an alias property for `window_id`. Keep one name. 5. **Duplicate-label averaging** (`analytics_service.py:1513-1526`). - Averaging is keyed by label, so a duplicated clock label blends both buckets: the test expects 15.0, where averaging by position gives 10.0. - The new guard already guarantees identical label sequences, so average by index. - The test pins an artefact: no real candidate shares a DST fold with the target. 6. **`_trusted_multiplier` (:83-87)** needs `# type: ignore[index]` only because narrowing goes through `_is_trusted_log`. Return the multiplier or None from one function. ## Spec **Pass-1 fix claims (c3431):** - #12: `active_business_day(span[0], reset)`. True. - #13: #203 exists, is open and `ready-for-agent`, cites #96/#167, and is linked from the plan doc. True. - #15: the plan doc is corrected (`domain_errors()`, "landed on master prior to branch", the #105/#106 move, #203). True. - #16: `_is_trusted_log`/`_trusted_multiplier` used at all three sites. True. - #11: the guard is present and correct, but the DST averaging is wrong (Sp1). - #14: partial (Sp2). **P3** 1. **Duplicate clock labels are pooled instead of averaged by position** (same as Standards P3-5). #97: *"Hourly baseline has no guard for differing bucket labels across source days."* 2. **#96's typed arguments are still partial.** #96 says: *"The cycles: list[tuple[str, float, float]] argument is an anonymous triple."* All four batch methods still accept `CycleWindow | tuple[str, float, float]` (`occupancy_repository.py:1646, 1684, 1721, 1789`). Only the returns gained `RangeCounts`/`CalibrationLogRecord`. Accept `CycleWindow` only. 3. **The `CycleWindow` unification is half done** (same as Standards P3-3). 4. **The PR description is stale.** - It still names `TimeRangeTarget`/`CycleQualityTarget` as the contracts and says "465 passed"; the fix reply says 526. - It doesn't mention `CycleWindow`, `RangeCounts`, `CalibrationLogRecord`, the bucket-label guard, or the #203 deferral. **Checklist:** - **#96:** the query count is partial, with the rest deferred to #203 (linked). Typed returns are done, but the triple argument remains. Everything else is done. - **#97:** done; the bucket-label guard has the averaging caveat. - **#124:** all done, so "Closes #124" is justified. ## Summary - **Standards:** 1 P1, 1 P2 and 4 P3s. The worst is the batched flow query, which is still 2.6–5x slower than per-day. - **Spec:** 4 P3s. The worst is the duplicate-label averaging. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg referenced this pull request from a commit 2026-10-02 16:36:20 +00:00

fix(statistics): address second pass review findings for PR #167

gabogg added 1 commit 2026-10-02 16:36:20 +00:00

fix(statistics): address second pass review findings for PR #167 38d128308b

gabogg commented 2026-10-02 16:37:52 +00:00

Author

Owner

Copy link

Pass 2 fixes (38d1283)

All 10 findings (1 P1, 1 P2, 8 P3s) from pass 2 review r22 have been resolved:

Standards

1. P1 — Batched flow query performance regression & unnested joins:
   • Rewrote get_counted_cycle_flow_range_async and get_counts_in_ranges_async in app/db/occupancy_repository.py by removing the parenthesized subquery join and joining people_counting_events directly per cycle/window with camera_index_code IN (SELECT ...).
   • Updated tests/test_statistics_periods.py to verify that EXPLAIN QUERY PLAN contains no MATERIALIZE, no SCAN (join-, and uses index search for both flow and open window queries.
2. P2 — CalibrationLogRecord schema alignment:
   • Derived CalibrationLogRecord in app/schemas/occupancy_models.py directly from occupancy_calibration_logs (app/db/database.py).
   • Removed non-table keys (previous_exit_multiplier, baseline_offset, patrol_guard_count), added real columns including target_guard_count, typed is_trusted and is_current as int matching SQLite return types, and removed total=False.
3. P3 — Speculative Generality & alias cleanup:
   • Removed deprecated TimeRangeTarget = CycleWindow and CycleQualityTarget = CycleWindow from occupancy_models.py.
   • Removed unused alias validate_closed_statistics_day_async from analytics_service.py.
   • Standardized on CycleWindow across all call sites in analytics_service.py and tests.
4. P3 — CycleWindow.cycle_date property:
   • Removed redundant cycle_date alias property from CycleWindow.
5. P3 — Duplicate-label averaging:
   • In get_hourly_timeseries_async (app/services/analytics_service.py), averaged usable samples by index position instead of grouping by clock label.
   • Updated test_hourly_baseline_appends_duplicate_clock_labels_on_dst_fallback in tests/test_statistics_baselines.py to assert positional averages (10.0 and 20.0).
6. P3 — _trusted_multiplier typing:
   • Extracted helper _trusted_cycle_multiplier(log) returning float | None, allowing clean narrowing in _is_trusted_log and _trusted_multiplier without # type: ignore[index].

Spec

1. P3 — Duplicate clock labels pooled instead of averaged by position:
   • Resolved alongside Standards P3-5.
2. P3 — Typed arguments for batch repository methods (#96):
   • Narrowed cycles / ranges parameter in all four batch repository methods (get_counted_cycle_flow_range_async, get_calibration_logs_for_cycles_async, get_counts_in_ranges_async, get_counted_cycle_quality_range_async) to Sequence[CycleWindow] only.
   • Updated callers and test seeds in tests/test_occupancy_config_api.py and tests/test_statistics_periods.py to pass CycleWindow.
3. P3 — CycleWindow unification:
   • Resolved alongside Standards P3-3.
4. P3 — Stale PR description:
   • Updated PR #167 description to reflect CycleWindow, RangeCounts, CalibrationLogRecord, positional baseline averaging, 526 passed tests, and linked follow-up issue #203.

All 526 tests passing, ruff lint/format clean, check_docs clean. Ready for pass 3 review.

## Pass 2 fixes (38d1283) All 10 findings (1 P1, 1 P2, 8 P3s) from pass 2 review r22 have been resolved: ### Standards 1. **P1 — Batched flow query performance regression & unnested joins**: - Rewrote `get_counted_cycle_flow_range_async` and `get_counts_in_ranges_async` in `app/db/occupancy_repository.py` by removing the parenthesized subquery join and joining `people_counting_events` directly per cycle/window with `camera_index_code IN (SELECT ...)`. - Updated `tests/test_statistics_periods.py` to verify that `EXPLAIN QUERY PLAN` contains no `MATERIALIZE`, no `SCAN (join-`, and uses index search for both flow and open window queries. 2. **P2 — CalibrationLogRecord schema alignment**: - Derived `CalibrationLogRecord` in `app/schemas/occupancy_models.py` directly from `occupancy_calibration_logs` (`app/db/database.py`). - Removed non-table keys (`previous_exit_multiplier`, `baseline_offset`, `patrol_guard_count`), added real columns including `target_guard_count`, typed `is_trusted` and `is_current` as `int` matching SQLite return types, and removed `total=False`. 3. **P3 — Speculative Generality & alias cleanup**: - Removed deprecated `TimeRangeTarget = CycleWindow` and `CycleQualityTarget = CycleWindow` from `occupancy_models.py`. - Removed unused alias `validate_closed_statistics_day_async` from `analytics_service.py`. - Standardized on `CycleWindow` across all call sites in `analytics_service.py` and tests. 4. **P3 — CycleWindow.cycle_date property**: - Removed redundant `cycle_date` alias property from `CycleWindow`. 5. **P3 — Duplicate-label averaging**: - In `get_hourly_timeseries_async` (`app/services/analytics_service.py`), averaged usable samples by index position instead of grouping by clock label. - Updated `test_hourly_baseline_appends_duplicate_clock_labels_on_dst_fallback` in `tests/test_statistics_baselines.py` to assert positional averages (10.0 and 20.0). 6. **P3 — _trusted_multiplier typing**: - Extracted helper `_trusted_cycle_multiplier(log)` returning `float | None`, allowing clean narrowing in `_is_trusted_log` and `_trusted_multiplier` without `# type: ignore[index]`. ### Spec 1. **P3 — Duplicate clock labels pooled instead of averaged by position**: - Resolved alongside Standards P3-5. 2. **P3 — Typed arguments for batch repository methods (#96)**: - Narrowed `cycles` / `ranges` parameter in all four batch repository methods (`get_counted_cycle_flow_range_async`, `get_calibration_logs_for_cycles_async`, `get_counts_in_ranges_async`, `get_counted_cycle_quality_range_async`) to `Sequence[CycleWindow]` only. - Updated callers and test seeds in `tests/test_occupancy_config_api.py` and `tests/test_statistics_periods.py` to pass `CycleWindow`. 3. **P3 — CycleWindow unification**: - Resolved alongside Standards P3-3. 4. **P3 — Stale PR description**: - Updated PR #167 description to reflect `CycleWindow`, `RangeCounts`, `CalibrationLogRecord`, positional baseline averaging, 526 passed tests, and linked follow-up issue #203. All 526 tests passing, ruff lint/format clean, check_docs clean. Ready for pass 3 review.

gabogg referenced this pull request 2026-10-02 23:51:06 +00:00

chore(statistics): PR #167 third-pass P3 follow-ups (multiplier helper names, typed log record, duplicated params and plan assertions) #219

gabogg reviewed 2026-10-02 23:51:07 +00:00

gabogg left a comment

Copy link

Code review, pass 3 (origin/master...38d1283, spec #96, #97, #124)

Result: no P1s, no P2s and 7 P3s. This is the third pass of a follow-up PR, so the P3s are deferred to the linked follow-up issue #219
#167 and the PR merges.

Verification:

• ruff: clean.
• test_statistics_periods, test_statistics_baselines and test_occupancy_config_api: 35 passed.
• Probes scratchpad/probe167p3*.py and spec167p3.py: SQLite 3.51.2, the real schema and indexes, 200k events, 20 cameras, 62 cycles. The table shows the minimum of 15 interleaved runs, in ms:

Shape	eda7191 (pass 2)	New batched (aiosqlite)	Per-day ×62 (aiosqlite)
365-day flow	229	59	68
365-day ranges	198	37	51
62-day flow	1255	442	392
62-day ranges	1529	216	210

Standards

Pass-2 fixes:

• P1, the batched queries: fixed.
  • Both get_counted_cycle_flow_range_async and get_counts_in_ranges_async now plan as SCAN p | SEARCH e USING INDEX idx_counting_events_cam_range (camera_index_code=? AND timestamp_epoch>? AND timestamp_epoch<?) LEFT-JOIN, with no MATERIALIZE and no SCAN (join-.
  • Totals are identical to the per-day path in both data shapes.
  • The new plan assertions fail on both the old unbounded and the old bounded query shapes.
  • Dense data is within 3–13% of per-day, against pass 2's 4–20× regression, so this no longer blocks the merge.
• P2, CalibrationLogRecord: fixed. All 26 columns are present with matching types, and is_trusted/is_current are int.
• P3s: fixed.
  • The aliases are gone.
  • Methods take CycleWindow only.
  • Labels are averaged by position, which is safe after the label guard; statistics_baseline_min_samples is at least 1.
  • _trusted_cycle_multiplier narrows without a type: ignore.

P3 (deferred to #219
#167):

1. The helper _trusted_cycle_multiplier and the method _trusted_cycle_multiplier_async have nearly the same name.
2. The helpers still take Mapping[str, Any], not CalibrationLogRecord.
3. CalibrationLogRecord omits rank_in_cycle, and the nullability of is_trusted doesn't match the schema.
4. The cycle-params comprehension is repeated four times in the repository.
5. The EXPLAIN-plan assertion block is repeated in the tests.

Spec

Fix claims (c3550): all true.

• S1–S6 are verified.
• Sp2: all four batch methods take Sequence[CycleWindow] only.
• Sp4: the live description names CycleWindow, RangeCounts, CalibrationLogRecord, positional averaging, 526 passed, and #203.

Checklist:

• #96: done. Per-day peak and average Riemann work is deferred to #203, which is linked.
• #97: every item is done; the daypart item moved to #105/#106.
• #124: done.

P3 (deferred to #219
#167):
6. The test name test_hourly_baseline_appends_duplicate_clock_labels_on_dst_fallback describes the old behaviour.
7. The PR addresses all of #97 but doesn't close it. I'm closing #97 by hand after the merge.

Summary

• Standards: 5 P3s. The worst is the near-duplicate multiplier helper names.
• Spec: 2 P3s. The worst is the stale test name.

Merging.

🤖 Generated with Claude Code

## Code review, pass 3 (`origin/master...38d1283`, spec #96, #97, #124) Result: **no P1s, no P2s and 7 P3s.** This is the third pass of a follow-up PR, so the P3s are deferred to the linked follow-up issue **#219 #167** and the PR **merges**. Verification: - ruff: clean. - `test_statistics_periods`, `test_statistics_baselines` and `test_occupancy_config_api`: 35 passed. - Probes `scratchpad/probe167p3*.py` and `spec167p3.py`: SQLite 3.51.2, the real schema and indexes, 200k events, 20 cameras, 62 cycles. The table shows the minimum of 15 interleaved runs, in ms: | Shape | eda7191 (pass 2) | New batched (aiosqlite) | Per-day ×62 (aiosqlite) | |---|---|---|---| | 365-day flow | 229 | **59** | 68 | | 365-day ranges | 198 | **37** | 51 | | 62-day flow | 1255 | 442 | **392** | | 62-day ranges | 1529 | 216 | **210** | ## Standards **Pass-2 fixes:** - **P1, the batched queries: fixed.** - Both `get_counted_cycle_flow_range_async` and `get_counts_in_ranges_async` now plan as `SCAN p | SEARCH e USING INDEX idx_counting_events_cam_range (camera_index_code=? AND timestamp_epoch>? AND timestamp_epoch<?) LEFT-JOIN`, with no `MATERIALIZE` and no `SCAN (join-`. - Totals are identical to the per-day path in both data shapes. - The new plan assertions fail on both the old unbounded and the old bounded query shapes. - Dense data is within 3–13% of per-day, against pass 2's 4–20× regression, so this no longer blocks the merge. - **P2, `CalibrationLogRecord`: fixed.** All 26 columns are present with matching types, and `is_trusted`/`is_current` are `int`. - **P3s: fixed.** - The aliases are gone. - Methods take `CycleWindow` only. - Labels are averaged by position, which is safe after the label guard; `statistics_baseline_min_samples` is at least 1. - `_trusted_cycle_multiplier` narrows without a `type: ignore`. **P3** (deferred to #219 #167): 1. The helper `_trusted_cycle_multiplier` and the method `_trusted_cycle_multiplier_async` have nearly the same name. 2. The helpers still take `Mapping[str, Any]`, not `CalibrationLogRecord`. 3. `CalibrationLogRecord` omits `rank_in_cycle`, and the nullability of `is_trusted` doesn't match the schema. 4. The cycle-params comprehension is repeated four times in the repository. 5. The EXPLAIN-plan assertion block is repeated in the tests. ## Spec **Fix claims (c3550):** all true. - S1–S6 are verified. - Sp2: all four batch methods take `Sequence[CycleWindow]` only. - Sp4: the live description names `CycleWindow`, `RangeCounts`, `CalibrationLogRecord`, positional averaging, 526 passed, and #203. **Checklist:** - **#96:** done. Per-day peak and average Riemann work is deferred to #203, which is linked. - **#97:** every item is done; the daypart item moved to #105/#106. - **#124:** done. **P3** (deferred to #219 #167): 6. The test name `test_hourly_baseline_appends_duplicate_clock_labels_on_dst_fallback` describes the old behaviour. 7. The PR addresses all of #97 but doesn't close it. I'm closing #97 by hand after the merge. ## Summary - **Standards:** 5 P3s. The worst is the near-duplicate multiplier helper names. - **Spec:** 2 P3s. The worst is the stale test name. Merging. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg merged commit 103157cf5e into master 2026-10-02 23:51:22 +00:00

gabogg deleted branch refactor/statistics-baseline-followups 2026-10-02 23:51:22 +00:00

gabogg referenced this pull request from a commit 2026-10-02 23:51:22 +00:00

refactor(statistics): baseline and query follow-ups (#167)

gabogg referenced this pull request 2026-10-02 23:52:23 +00:00

follow-up(statistics): P3 cleanups from #90 review (usual-weekday baselines) #97

gabogg referenced this pull request 2026-10-02 23:52:26 +00:00

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

gabogg referenced this pull request 2026-10-03 07:18:52 +00:00

refactor(statistics): third-pass P3 follow-ups from PRs #167 and #168 #227

gabogg referenced this pull request 2026-10-03 07:24:32 +00:00

feat(schedule): activate manual calibration and time changes next day #177

gabogg referenced this pull request from a commit 2026-10-03 07:45:12 +00:00

chore(statistics): address third-pass P3 follow-ups from PRs #167 and #168

gabogg referenced this pull request 2026-10-03 07:59:35 +00:00

research(docs): reinforce documentation, enforce its maintenance under agentic development, and shape a knowledge base for agents #208

gabogg referenced this pull request 2026-10-03 08:04:47 +00:00

refactor(statistics): third-pass P3 follow-ups from PRs #167 and #168 #227

gabogg referenced this pull request from a commit 2026-10-03 12:58:32 +00:00

refactor(statistics): third-pass P3 follow-ups from PRs #167 and #168 (#227)

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

No description provided.