feat(statistics): usual weekday baseline matches opening hours; configurable sampling (#105) #122

Merged

gabogg merged 2 commits from feat/statistics-baseline-rules into master 2026-09-25 22:13:20 +00:00

Conversation 4 Commits 2 Files changed 16 +589 -80

gabogg commented 2026-09-25 19:46:48 +00:00

Owner

Copy link

Closes #105

Summary

Step 3 of the pre-deck work (2026-09-25 decisions). The Usual Weekday Baseline:

• only compares a day with earlier days that had the same opening hours;
• its sampling (max days, lookback weeks, minimum days) is configured per facility instead of hard-coded at 4 / 8 / 2;
• its route parameter is renamed to baseline=usual_weekday.

Architectural impact

• One opening-hours rule. opening_hours_by_schedule(exception, weekday, holiday_hours) in occupancy_service.py is used by get_active_schedule_info_async and by ScheduleCalendar.opening_hours (the calendar now carries the configured holiday hours). This mirrors is_open_by_schedule from #110. The weekday fallback hours are named constants instead of literals.
• Baseline selection. get_usual_weekday_baseline_dates_async(day, reset, rules, calendar) takes a UsualWeekdayRules(max_samples, lookback_weeks, min_samples) built from the config. It skips candidates whose opening hours differ from the target day's. All three callers (summary, presenter/admin hourly, dayparts) use rules.min_samples.
• Config. New keys statistics_baseline_max_samples (4), statistics_baseline_lookback_weeks (8) and statistics_baseline_min_samples (2):
  • named defaults and a BASELINE_LOOKBACK_WEEKS_LIMIT of 52;
  • added by the in-place column migration;
  • editable in the admin form, labels in es and en.
• Validation. OccupancyConfigSchema enforces min ≤ max ≤ lookback. POST /api/occupancy/config now validates the merged configuration before a partial save, so a save that sends one field can't break the rule (returns 422 VALIDATION_ERROR).
• Contract. UsualWeekdayBaselineKind = Literal["usual_weekday"] on the four routes. same_weekday_4w is now rejected; no client used it yet.
• Docs. Glossary wording as agreed in #105, with no numbers. The API README documents the settings and the hours rule.

Known limitation: schedules have no history yet, so past days are classified with the current opening hours. Until #113 lands, the hours rule only excludes days when the target day is a dated exception with its own hours.

Verification

• New tests/test_statistics_baseline_rules.py (8 tests):
  • the default rules, and narrower max and lookback settings;
  • the configured minimum decides whether a baseline exists (through the config API and the hourly route);
  • a target day with exception hours has no usual days, and one with the weekday's own hours matches again;
  • four inconsistent or out-of-range settings are rejected and leave the stored config unchanged;
  • the retired same_weekday_4w value is rejected.
• The existing baseline and closed-day tests are updated for the rename and the new signature.
• pytest: 362 passed, 1 skipped. Frontend: 80/80. Ruff, pre-commit and scripts/check_docs.py passed.

Checklist

• Same-opening-hours rule, shared with the live schedule.
• Configurable max, lookback and min with named defaults; consistency validated on save.
• baseline=usual_weekday on all four routes.
• Glossary and API docs.

🤖 Generated with Claude Code

Closes #105 ## Summary Step 3 of the pre-deck work (2026-09-25 decisions). The Usual Weekday Baseline: - only compares a day with earlier days that had the **same opening hours**; - its sampling (max days, lookback weeks, minimum days) is **configured per facility** instead of hard-coded at 4 / 8 / 2; - its route parameter is renamed to `baseline=usual_weekday`. ## Architectural impact - **One opening-hours rule.** `opening_hours_by_schedule(exception, weekday, holiday_hours)` in `occupancy_service.py` is used by `get_active_schedule_info_async` and by `ScheduleCalendar.opening_hours` (the calendar now carries the configured holiday hours). This mirrors `is_open_by_schedule` from #110. The weekday fallback hours are named constants instead of literals. - **Baseline selection.** `get_usual_weekday_baseline_dates_async(day, reset, rules, calendar)` takes a `UsualWeekdayRules(max_samples, lookback_weeks, min_samples)` built from the config. It skips candidates whose opening hours differ from the target day's. All three callers (summary, presenter/admin hourly, dayparts) use `rules.min_samples`. - **Config.** New keys `statistics_baseline_max_samples` (4), `statistics_baseline_lookback_weeks` (8) and `statistics_baseline_min_samples` (2): - named defaults and a `BASELINE_LOOKBACK_WEEKS_LIMIT` of 52; - added by the in-place column migration; - editable in the admin form, labels in es and en. - **Validation.** `OccupancyConfigSchema` enforces min ≤ max ≤ lookback. `POST /api/occupancy/config` now validates the **merged** configuration before a partial save, so a save that sends one field can't break the rule (returns `422 VALIDATION_ERROR`). - **Contract.** `UsualWeekdayBaselineKind = Literal["usual_weekday"]` on the four routes. `same_weekday_4w` is now rejected; no client used it yet. - **Docs.** Glossary wording as agreed in #105, with no numbers. The API README documents the settings and the hours rule. **Known limitation:** schedules have no history yet, so past days are classified with the **current** opening hours. Until #113 lands, the hours rule only excludes days when the target day is a dated exception with its own hours. ## Verification - New `tests/test_statistics_baseline_rules.py` (8 tests): - the default rules, and narrower max and lookback settings; - the configured minimum decides whether a baseline exists (through the config API and the hourly route); - a target day with exception hours has no usual days, and one with the weekday's own hours matches again; - four inconsistent or out-of-range settings are rejected and leave the stored config unchanged; - the retired `same_weekday_4w` value is rejected. - The existing baseline and closed-day tests are updated for the rename and the new signature. - `pytest`: 362 passed, 1 skipped. Frontend: 80/80. Ruff, pre-commit and `scripts/check_docs.py` passed. ## Checklist - [x] Same-opening-hours rule, shared with the live schedule. - [x] Configurable max, lookback and min with named defaults; consistency validated on save. - [x] `baseline=usual_weekday` on all four routes. - [x] Glossary and API docs. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg added 1 commit 2026-09-25 19:46:49 +00:00

feat(statistics): usual weekday baseline matches opening hours; configurable sampling ... 6f0882ff8f

- The Usual Weekday Baseline only uses earlier days whose opening hours
  (after dated-exception overrides) match the target day's. One rule,
  `opening_hours_by_schedule`, now serves the live schedule and the
  `ScheduleCalendar` used by statistics.
- Sampling is configured per facility instead of fixed at 4 / 8 / 2:
  `statistics_baseline_max_samples`, `_lookback_weeks` and `_min_samples`
  (named defaults and a 52-week limit). A save must keep
  min <= max <= lookback, checked on the merged configuration.
- The query value is renamed from `baseline=same_weekday_4w` to
  `baseline=usual_weekday` on all four routes; the old value is rejected.
- Glossary wording for Usual Weekday Baseline has no numbers; the admin
  form gets the three settings (es/en).

Opening hours come from the current schedule until per-day schedule
records exist (#113).

Closes #105

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

gabogg commented 2026-09-25 19:52:42 +00:00

Author

Owner

Copy link

Code review — pass 1 (two-axis)

Reviewed git diff origin/master...origin/feat/statistics-baseline-rules (6f0882f) against #105. Standards and Spec ran independently and are reported separately. Severity: P1 must fix · P2 fix before merge · P3 minor. Per review policy, pass 1 findings are all fixed on the branch.

Standards

No hard violations of the documented standards. Ruff clean; the three baseline and closed-day test files pass (25). The 422 follows the {detail, error_code} contract (main.py:95 maps X-Error-Code to VALIDATION_ERROR).

Bugs and risks

• P2 app/controllers/occupancy_controller.py:127-135: read config → validate merged → write is not atomic. Two concurrent admin saves can each pass and still store rows breaking min ≤ max ≤ lookback (A sets min=4 while B sets max=3). Then GET /config (:108) and the POST's own response (:135) raise an unhandled ValidationError → 500, and the admin form can only be fixed in the DB. Old rows are safe (migration defaults 4/8/2 are consistent). Fix: validate inside the repository write transaction, or make GET degrade gracefully.
• P3 occupancy_controller.py:131: detail becomes "Value error, usual weekday baseline needs…" — Pydantic's prefix leaks, and only the first error is reported.
• P3 app/services/analytics_service.py:213-231, performance: each surviving candidate costs 3 sequential queries (flow, _cycle_quality_async, trusted). With a 52-week lookback and sparse clean data that's up to ~156 queries per call, and the summary re-queries each selected day's flow. get_counted_cycle_quality_range_async already takes a batch.
• P3 occupancy_service.py:135, ScheduleCalendar.holiday_hours default: constructing without it silently uses 10:00–18:00 instead of the configured hours. The only constructor (:276) passes it, so the default is Speculative Generality that can hide bugs. Make it required.
• No behaviour change in opening_hours_by_schedule vs the old live-schedule code: holidays use custom hours else the configured holiday hours; weekdays use the row else 08:00–21:00; None passes through as before.
• Note, analytics_service.py:218: candidates with an exception are skipped earlier, so opening_hours(candidate) is always the same weekday's hours; the check only matters when the target is an exception (as the PR says) and could be computed once outside the loop.

Smells (judgement calls)

• Magic numbers / Shotgun Surgery: defaults 4, 8, 2 and limit 52 appear in five places (named constants, migration literals database.py:248-250, from_config fallbacks, index.html:805-809 min/max/value). Holiday hours "10:00"/"18:00" are still literals in occupancy_repository.py:276-292,335,404,523 next to the new DEFAULT_HOLIDAY_HOURS. The migration could interpolate the constants.
• Misplaced constants: DEFAULT_WEEKDAY_HOURS / DEFAULT_HOLIDAY_HOURS (occupancy_service.py:99-100) sit in the service layer, while other DEFAULT_* live in schemas/occupancy_models.py (the repository can't import from services).
• Primitive Obsession: opening hours travel as tuple[str, str] read by [0]/[1]; a small OpeningHours NamedTuple would name the parts.
• Speculative Generality: the .get(..., DEFAULT) fallbacks in configured_holiday_hours and UsualWeekdayRules.from_config can't trigger (the repository always fills these keys).

Spec

Checked against every settled decision in #105. The glossary text matches the spec word for word, all four routes are renamed, the defaults stay 4/8/2, and nothing in the frontend or docs still uses same_weekday_4w, "four" or "eight weeks". One bug and one missing doc item.

(c) Implemented but wrong

• P2: the daypart baseline still has a hard-coded minimum of 2 (app/services/analytics_service.py:955: if len(usable) < 2:). Spec: "Replace the fixed numbers with config keys (no magic numbers): … minimum samples (default 2)." and "so the hourly and daypart baselines stay consistent." The first check (:935) uses rules.min_samples; the second, after dropping sample days with no entries, still uses 2. With min_samples=1, dayparts returns INSUFFICIENT_USUAL_WEEKDAYS where hourly returns a baseline; with min_samples=3 and one sample dropped, dayparts builds a baseline from 2 days. Use rules.min_samples and add a daypart test with a non-default minimum (the only minimum test goes through hourly, tests/test_statistics_baseline_rules.py:104).

(a) Missing or partial

• P2: dwell weighting dropped from the docs entirely. Spec: "Dwell weighting and the volume-daypart rule stay in the API docs, not the glossary." CONTEXT.md:97 correctly drops "Daypart dwell is weighted by each day's entries", but docs/api/README.md:128 never picked it up — it has the volume-daypart rule but nothing on weighting by entries.

(b) Scope creep / agent-made decisions (for audit)

• P3: rules the spec did not settle: min ≤ max ≤ lookback validated on the merged config on every POST /api/occupancy/config (occupancy_controller.py:123); the 52-week limit (BASELINE_LOOKBACK_WEEKS_LIMIT, occupancy_models.py:208); admin form fields and es/en labels (index.html:804). Reasonable, but confirm them explicitly.

Stated limitation (#113)

• P3: stated accurately, but it matters more than the wording suggests. Candidates with a dated exception are already excluded, so every remaining candidate resolves to the current weekday hours; the hours rule (analytics_service.py:218) only changes anything when the target day is itself an exception, and the test (test_statistics_baseline_rules.py:147) can only cover that case. Make #113 name ScheduleCalendar.opening_hours as the call site to switch to per-day records.

Confirmed correct

• Glossary matches the spec's text, including _Avoid_.
• All four routes accept only baseline=usual_weekday (presenter/admin hourly and dayparts covered in tests/test_statistics_baselines.py); the retired value is rejected (:207).
• One shared date list and ScheduleCalendar (now carrying configured holiday hours) for summary, hourly and dayparts.
• Defaults 4/8/2 kept in the named constants, migration and repository defaults.
• No leftovers in app/, docs/, tests/, CONTEXT.md (the range(1, 9) in tests/test_statistics_baselines.py:64 only seeds data).
• New tests fail without the change; the only uncovered claimed behaviour is the daypart minimum (the P2 above).

---

Summary: Standards 10 (1×P2, 9×P3), worst: concurrent config saves can store an inconsistent baseline rule, after which GET /config returns 500. Spec 5 (2×P2, 3×P3), worst: the daypart baseline still has a hard-coded minimum of 2.

🤖 Generated with Claude Code

# Code review — pass 1 (two-axis) Reviewed `git diff origin/master...origin/feat/statistics-baseline-rules` (6f0882f) against #105. Standards and Spec ran independently and are reported separately. Severity: **P1** must fix · **P2** fix before merge · **P3** minor. Per review policy, pass 1 findings are all fixed on the branch. ## Standards No hard violations of the documented standards. Ruff clean; the three baseline and closed-day test files pass (25). The 422 follows the `{detail, error_code}` contract (`main.py:95` maps `X-Error-Code` to `VALIDATION_ERROR`). ### Bugs and risks - **P2 `app/controllers/occupancy_controller.py:127-135`**: read config → validate merged → write is not atomic. Two concurrent admin saves can each pass and still store rows breaking min ≤ max ≤ lookback (A sets min=4 while B sets max=3). Then `GET /config` (`:108`) and the POST's own response (`:135`) raise an unhandled `ValidationError` → 500, and the admin form can only be fixed in the DB. Old rows are safe (migration defaults 4/8/2 are consistent). Fix: validate inside the repository write transaction, or make GET degrade gracefully. - **P3 `occupancy_controller.py:131`**: `detail` becomes "Value error, usual weekday baseline needs…" — Pydantic's prefix leaks, and only the first error is reported. - **P3 `app/services/analytics_service.py:213-231`, performance**: each surviving candidate costs 3 sequential queries (flow, `_cycle_quality_async`, trusted). With a 52-week lookback and sparse clean data that's up to ~156 queries per call, and the summary re-queries each selected day's flow. `get_counted_cycle_quality_range_async` already takes a batch. - **P3 `occupancy_service.py:135`, `ScheduleCalendar.holiday_hours` default**: constructing without it silently uses 10:00–18:00 instead of the configured hours. The only constructor (`:276`) passes it, so the default is Speculative Generality that can hide bugs. Make it required. - **No behaviour change** in `opening_hours_by_schedule` vs the old live-schedule code: holidays use custom hours else the configured holiday hours; weekdays use the row else 08:00–21:00; `None` passes through as before. - **Note, `analytics_service.py:218`**: candidates with an exception are skipped earlier, so `opening_hours(candidate)` is always the same weekday's hours; the check only matters when the target is an exception (as the PR says) and could be computed once outside the loop. ### Smells (judgement calls) - **Magic numbers / Shotgun Surgery**: defaults 4, 8, 2 and limit 52 appear in five places (named constants, migration literals `database.py:248-250`, `from_config` fallbacks, `index.html:805-809` `min`/`max`/`value`). Holiday hours `"10:00"`/`"18:00"` are still literals in `occupancy_repository.py:276-292,335,404,523` next to the new `DEFAULT_HOLIDAY_HOURS`. The migration could interpolate the constants. - **Misplaced constants**: `DEFAULT_WEEKDAY_HOURS` / `DEFAULT_HOLIDAY_HOURS` (`occupancy_service.py:99-100`) sit in the service layer, while other `DEFAULT_*` live in `schemas/occupancy_models.py` (the repository can't import from services). - **Primitive Obsession**: opening hours travel as `tuple[str, str]` read by `[0]`/`[1]`; a small `OpeningHours` NamedTuple would name the parts. - **Speculative Generality**: the `.get(..., DEFAULT)` fallbacks in `configured_holiday_hours` and `UsualWeekdayRules.from_config` can't trigger (the repository always fills these keys). ## Spec Checked against every settled decision in #105. The glossary text matches the spec word for word, all four routes are renamed, the defaults stay 4/8/2, and nothing in the frontend or docs still uses `same_weekday_4w`, "four" or "eight weeks". One bug and one missing doc item. ### (c) Implemented but wrong - **P2: the daypart baseline still has a hard-coded minimum of 2** (`app/services/analytics_service.py:955`: `if len(usable) < 2:`). Spec: *"Replace the fixed numbers with config keys (no magic numbers): … minimum samples (default 2)."* and *"so the hourly and daypart baselines stay consistent."* The first check (`:935`) uses `rules.min_samples`; the second, after dropping sample days with no entries, still uses 2. With `min_samples=1`, dayparts returns `INSUFFICIENT_USUAL_WEEKDAYS` where hourly returns a baseline; with `min_samples=3` and one sample dropped, dayparts builds a baseline from 2 days. Use `rules.min_samples` and add a daypart test with a non-default minimum (the only minimum test goes through hourly, `tests/test_statistics_baseline_rules.py:104`). ### (a) Missing or partial - **P2: dwell weighting dropped from the docs entirely.** Spec: *"Dwell weighting and the volume-daypart rule stay in the API docs, not the glossary."* `CONTEXT.md:97` correctly drops "Daypart dwell is weighted by each day's entries", but `docs/api/README.md:128` never picked it up — it has the volume-daypart rule but nothing on weighting by entries. ### (b) Scope creep / agent-made decisions (for audit) - **P3: rules the spec did not settle:** min ≤ max ≤ lookback validated on the merged config on every `POST /api/occupancy/config` (`occupancy_controller.py:123`); the 52-week limit (`BASELINE_LOOKBACK_WEEKS_LIMIT`, `occupancy_models.py:208`); admin form fields and es/en labels (`index.html:804`). Reasonable, but confirm them explicitly. ### Stated limitation (#113) - **P3:** stated accurately, but it matters more than the wording suggests. Candidates with a dated exception are already excluded, so every remaining candidate resolves to the current weekday hours; the hours rule (`analytics_service.py:218`) only changes anything when the target day is itself an exception, and the test (`test_statistics_baseline_rules.py:147`) can only cover that case. Make #113 name `ScheduleCalendar.opening_hours` as the call site to switch to per-day records. ### Confirmed correct - Glossary matches the spec's text, including `_Avoid_`. - All four routes accept only `baseline=usual_weekday` (presenter/admin hourly and dayparts covered in `tests/test_statistics_baselines.py`); the retired value is rejected (`:207`). - One shared date list and `ScheduleCalendar` (now carrying configured holiday hours) for summary, hourly and dayparts. - Defaults 4/8/2 kept in the named constants, migration and repository defaults. - No leftovers in `app/`, `docs/`, `tests/`, `CONTEXT.md` (the `range(1, 9)` in `tests/test_statistics_baselines.py:64` only seeds data). - New tests fail without the change; the only uncovered claimed behaviour is the daypart minimum (the P2 above). --- **Summary:** Standards 10 (1×P2, 9×P3), worst: concurrent config saves can store an inconsistent baseline rule, after which GET /config returns 500. Spec 5 (2×P2, 3×P3), worst: the daypart baseline still has a hard-coded minimum of 2. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg commented 2026-09-25 21:46:42 +00:00

Author

Owner

Copy link

Pass-1 findings addressed in the latest commit. Full suite 364 passed, 1 skipped; frontend 80/80; ruff and docs check clean.

Agent-made decisions confirmed by the maintainer: min ≤ max ≤ lookback validation on the merged config, the 52-week lookback limit, and the three admin form fields with es/en labels.

Standards

Finding	Fix
P2 read-validate-write race → inconsistent rows → GET 500	update_config_fields_async now does BEGIN IMMEDIATE, reads the stored row, validates the merged configuration with OccupancyConfigSchema, then writes, all in one transaction. The controller only maps ValidationError to 422. New test_concurrent_saves_cannot_store_inconsistent_sampling_rules: two saves that are valid alone but not together → exactly one refused, stored rules consistent.
P3 Pydantic prefix / first error only	The 422 detail joins every error message with the "Value error, " prefix removed.
P3 up to ~156 queries per selection	Calendar filters first; counted data and gap flags for all candidates come from one get_counted_cycle_quality_range_async call; the trust verdict is read per candidate only until max_samples pass.
P3 ScheduleCalendar.holiday_hours default	Required field.
Hours check per candidate	Kept inside the calendar filter; it is an in-memory lookup (no query), and it becomes meaningful per candidate once #113 adds per-day records.
Magic numbers / Shotgun Surgery	The migration interpolates the named defaults; the repository's "10:00"/"18:00" literals now use DEFAULT_HOLIDAY_HOURS. The HTML min/max/value attributes stay literal, as with the other config inputs (HTML can't import them).
Misplaced constants	DEFAULT_WEEKDAY_HOURS / DEFAULT_HOLIDAY_HOURS moved to schemas/occupancy_models.py.
Primitive Obsession tuple[str, str]	OpeningHours(opens, closes) NamedTuple.
Speculative Generality .get fallbacks	configured_holiday_hours and UsualWeekdayRules.from_config read the keys directly; tests use UsualWeekdayRules.defaults().

Spec

Finding	Fix
P2 daypart baseline hard-coded minimum 2	Uses rules.min_samples. New test_hourly_and_daypart_baselines_use_the_same_configured_minimum (min 1, one usable day → baseline on both routes); fails with the hard-coded 2 (mutation-checked).
P2 dwell weighting missing from docs	API README: "Daypart dwell in the baseline is weighted by each source day's entries".
P3 agent-made decisions	Confirmed by the maintainer (see above).
P3 #113 call site	#113 now names ScheduleCalendar.opening_hours (and is_closed) as the place to switch to per-day records.

🤖 Generated with Claude Code

Pass-1 findings addressed in the latest commit. Full suite 364 passed, 1 skipped; frontend 80/80; ruff and docs check clean. **Agent-made decisions confirmed by the maintainer:** min ≤ max ≤ lookback validation on the merged config, the 52-week lookback limit, and the three admin form fields with es/en labels. ### Standards | Finding | Fix | |---|---| | P2 read-validate-write race → inconsistent rows → GET 500 | `update_config_fields_async` now does `BEGIN IMMEDIATE`, reads the stored row, validates the merged configuration with `OccupancyConfigSchema`, then writes, all in one transaction. The controller only maps `ValidationError` to 422. New `test_concurrent_saves_cannot_store_inconsistent_sampling_rules`: two saves that are valid alone but not together → exactly one refused, stored rules consistent. | | P3 Pydantic prefix / first error only | The 422 `detail` joins every error message with the "Value error, " prefix removed. | | P3 up to ~156 queries per selection | Calendar filters first; counted data and gap flags for all candidates come from one `get_counted_cycle_quality_range_async` call; the trust verdict is read per candidate only until `max_samples` pass. | | P3 `ScheduleCalendar.holiday_hours` default | Required field. | | Hours check per candidate | Kept inside the calendar filter; it is an in-memory lookup (no query), and it becomes meaningful per candidate once #113 adds per-day records. | | Magic numbers / Shotgun Surgery | The migration interpolates the named defaults; the repository's `"10:00"`/`"18:00"` literals now use `DEFAULT_HOLIDAY_HOURS`. The HTML `min`/`max`/`value` attributes stay literal, as with the other config inputs (HTML can't import them). | | Misplaced constants | `DEFAULT_WEEKDAY_HOURS` / `DEFAULT_HOLIDAY_HOURS` moved to `schemas/occupancy_models.py`. | | Primitive Obsession `tuple[str, str]` | `OpeningHours(opens, closes)` NamedTuple. | | Speculative Generality `.get` fallbacks | `configured_holiday_hours` and `UsualWeekdayRules.from_config` read the keys directly; tests use `UsualWeekdayRules.defaults()`. | ### Spec | Finding | Fix | |---|---| | P2 daypart baseline hard-coded minimum 2 | Uses `rules.min_samples`. New `test_hourly_and_daypart_baselines_use_the_same_configured_minimum` (min 1, one usable day → baseline on both routes); fails with the hard-coded 2 (mutation-checked). | | P2 dwell weighting missing from docs | API README: "Daypart dwell in the baseline is weighted by each source day's entries". | | P3 agent-made decisions | Confirmed by the maintainer (see above). | | P3 #113 call site | #113 now names `ScheduleCalendar.opening_hours` (and `is_closed`) as the place to switch to per-day records. | 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg referenced this pull request 2026-09-25 21:46:43 +00:00

feat(schedule): each business day remembers the schedule it was measured under #113

gabogg added 1 commit 2026-09-25 21:46:44 +00:00

fix(statistics): address review of the baseline rules ... 67a37e3e81

- Config saves validate the merged configuration inside one
  `BEGIN IMMEDIATE` transaction in the repository, so concurrent saves
  cannot together store rules that break min <= max <= lookback; the 422
  message no longer carries Pydantic's "Value error" prefix and lists every
  error.
- The daypart baseline uses the configured minimum after dropping empty
  sample days, like the hourly baseline.
- Candidate days are checked for counted data and gaps in one batched
  query; trust is read only until enough days pass.
- `OpeningHours` names the (opens, closes) pair; the weekday and holiday
  fallbacks live with the other defaults in `occupancy_models.py`, and the
  repository and migration use the named constants instead of literals.
  `ScheduleCalendar.holiday_hours` is required and the rules are read from
  the config without silent fallbacks (`UsualWeekdayRules.defaults()`).
- API docs state that daypart dwell is weighted by entries.

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

gabogg commented 2026-09-25 22:11:35 +00:00

Author

Owner

Copy link

Code review — pass 2 (two-axis)

Re-reviewed at 67a37e3. Each axis verified every pass-1 finding against the code, then reviewed the fix commit for new problems. Verdicts: ✅ fixed · ◐ partial · ❌ not addressed · ↺ regressed · ⊘ declined with documented reason. Per policy, P1/P2 block merge; P3s go to one follow-up issue.

Standards

Pass-1 findings

Finding	Verdict	Evidence (67a37e3)
P2 read→validate→write race	✅ fixed	occupancy_repository.py:467-482: BEGIN IMMEDIATE, read row, validate merged with OccupancyConfigSchema, UPDATE, commit in one transaction; rollback on BaseException. In sqlite3's legacy transaction mode the explicit BEGIN means no implicit BEGIN before the UPDATE; the get_async_db PRAGMAs don't open a transaction; a second writer waits on busy_timeout=5000 instead of an immediate SQLITE_BUSY. Mutation-checked: removing BEGIN IMMEDIATE, or moving validation back before the transaction, fails test_concurrent_saves_cannot_store_inconsistent_sampling_rules 3/3; with the fix the file passes 5/5.
P3 Pydantic prefix / first error only	✅ fixed	occupancy_controller.py:127-133
P3 per-candidate queries	◐ partial	analytics_service.py:222-243: one batched quality read, then one trust query per candidate until max_samples pass (up to lookback_weeks when many days are untrusted). The summary still re-reads each selected day's flow (:452-457). Not blocking.
P3 ScheduleCalendar.holiday_hours default	✅ fixed	occupancy_service.py:128
Note: hours check per candidate	⊘ declined-with-reason	In-memory lookup; meaningful with #113.
Magic numbers / Shotgun Surgery	◐ partial	Migration interpolates the constants (database.py:253-268); repository literals use DEFAULT_HOLIDAY_HOURS; HTML kept literal with a reason. The seed row still hard-codes '10:00', '18:00' (database.py:284) and the weekday seed "08:00", "21:00" (:300-306) next to DEFAULT_WEEKDAY_HOURS.
Misplaced constants	✅ fixed	schemas/occupancy_models.py:205-215
Primitive Obsession tuple[str,str]	✅ fixed	OpeningHours, used through occupancy_service.py:99-132
Speculative Generality .get fallbacks	✅ fixed, no KeyError risk	Every caller gets cfg from get_config_async (migrated row + setdefaults; the no-row fallback dict has every key); the only test faking get_config_async never reaches these functions; tests use UsualWeekdayRules.defaults().

New findings

• P3 occupancy_repository.py:1449: get_counted_cycle_quality_range_async bounds its audit CTE with BETWEEN cycles[0][0] AND cycles[-1][0], silently requiring oldest-first input. Only a caller comment records it (analytics_service.py:221); correct today via sorted(candidates), but a future newest-first caller would silently lose audit rows. Use min/max in the repository or document the precondition.
• P3 occupancy_repository.py:472: dict(stored) raises TypeError → 500 if row 1 is missing (the old UPDATE was a no-op). init_schema always seeds the row, so theoretical.
• P3 (possible Duplicated Code): the seed literals in database.py:284,300-306 duplicate DEFAULT_HOLIDAY_HOURS / DEFAULT_WEEKDAY_HOURS.
• No standards breach in the fix commit. Ruff clean; full suite 364 passed, 1 skipped.

Merge readiness (this axis): Ready. P3s → follow-up issue.

Spec

Fix commit 67a37e3 checked against pass-1 head 6f0882f and #105. The three baseline test files pass (27) in a throwaway worktree; mutation check: restoring the daypart minimum to < 2 fails test_hourly_and_daypart_baselines_use_the_same_configured_minimum.

Pass-1 findings

Finding	Verdict	Evidence
P2 daypart baseline hard-coded minimum 2	✅ fixed	analytics_service.py:965 is if len(usable) < rules.min_samples:; summary (:446), hourly (:1216) and daypart (:945, :965) all use rules.min_samples. Test at tests/test_statistics_baseline_rules.py:220 (min 1, one usable day → baseline on both routes) fails with the old 2.
P2 dwell weighting missing from API docs	✅ fixed	docs/api/README.md:128: "Daypart dwell in the baseline is weighted by each source day's entries (Σ dwell × entries / Σ entries)", matching analytics_service.py:985-989; the glossary still leaves it out, as specified.
P3 unsettled rules (min ≤ max ≤ lookback, 52-week limit, admin fields)	✅ resolved	Confirmed by the maintainer; merged-config validation now runs inside the write transaction (occupancy_repository.py:449-483), covered by test_concurrent_saves_cannot_store_inconsistent_sampling_rules.
P3 #113 should name the call site	✅ fixed	#113 comment names ScheduleCalendar.opening_hours(day), ScheduleCalendar.is_closed(day) and get_active_schedule_info_async, and restates the limitation.

New findings

None at P1/P2. The reworked candidate selection keeps every source-day condition in the glossary ("A day counts only if it has counted flow, is not a holiday or Closed Day, has no ingestion gap, is trusted in its current audit, and had the same opening hours."):

• Calendar checks (analytics_service.py:209-219: holiday, Closed Day, same hours, before the current business day) still run before any database read.
• Counted flow and gaps come from one batched get_counted_cycle_quality_range_async read (occupancy_repository.py:1441): has_data (EXISTS on counted-camera events) matches the old COUNT(*) > 0; has_gap uses the same overlap rule as the old _cycle_quality_async; candidates are sorted oldest first for the query's BETWEEN.
• Trust is still read per candidate, newest first, stopping at max_samples — same selected days and order as before.

Glossary, the rename to baseline=usual_weekday on all four routes and the 4/8/2 defaults are unchanged; the migration now builds its defaults from the named constants.

Merge readiness (this axis): Ready.

---

Summary: Standards: no P1/P2; 3 P3 (worst: hidden oldest-first precondition in the batched quality query). Spec: no findings; ready.

🤖 Generated with Claude Code

# Code review — pass 2 (two-axis) Re-reviewed at `67a37e3`. Each axis verified every pass-1 finding against the code, then reviewed the fix commit for new problems. Verdicts: ✅ fixed · ◐ partial · ❌ not addressed · ↺ regressed · ⊘ declined with documented reason. Per policy, P1/P2 block merge; P3s go to one follow-up issue. ## Standards ### Pass-1 findings | Finding | Verdict | Evidence (67a37e3) | |---|---|---| | P2 read→validate→write race | ✅ fixed | `occupancy_repository.py:467-482`: `BEGIN IMMEDIATE`, read row, validate merged with `OccupancyConfigSchema`, UPDATE, commit in one transaction; rollback on `BaseException`. In sqlite3's legacy transaction mode the explicit BEGIN means no implicit BEGIN before the UPDATE; the `get_async_db` PRAGMAs don't open a transaction; a second writer waits on `busy_timeout=5000` instead of an immediate SQLITE_BUSY. **Mutation-checked:** removing `BEGIN IMMEDIATE`, or moving validation back before the transaction, fails `test_concurrent_saves_cannot_store_inconsistent_sampling_rules` 3/3; with the fix the file passes 5/5. | | P3 Pydantic prefix / first error only | ✅ fixed | `occupancy_controller.py:127-133` | | P3 per-candidate queries | ◐ partial | `analytics_service.py:222-243`: one batched quality read, then one trust query per candidate until `max_samples` pass (up to `lookback_weeks` when many days are untrusted). The summary still re-reads each selected day's flow (`:452-457`). Not blocking. | | P3 `ScheduleCalendar.holiday_hours` default | ✅ fixed | `occupancy_service.py:128` | | Note: hours check per candidate | ⊘ declined-with-reason | In-memory lookup; meaningful with #113. | | Magic numbers / Shotgun Surgery | ◐ partial | Migration interpolates the constants (`database.py:253-268`); repository literals use `DEFAULT_HOLIDAY_HOURS`; HTML kept literal with a reason. The seed row still hard-codes `'10:00', '18:00'` (`database.py:284`) and the weekday seed `"08:00", "21:00"` (`:300-306`) next to `DEFAULT_WEEKDAY_HOURS`. | | Misplaced constants | ✅ fixed | `schemas/occupancy_models.py:205-215` | | Primitive Obsession `tuple[str,str]` | ✅ fixed | `OpeningHours`, used through `occupancy_service.py:99-132` | | Speculative Generality `.get` fallbacks | ✅ fixed, no KeyError risk | Every caller gets `cfg` from `get_config_async` (migrated row + `setdefault`s; the no-row fallback dict has every key); the only test faking `get_config_async` never reaches these functions; tests use `UsualWeekdayRules.defaults()`. | ### New findings - **P3 `occupancy_repository.py:1449`**: `get_counted_cycle_quality_range_async` bounds its audit CTE with `BETWEEN cycles[0][0] AND cycles[-1][0]`, silently requiring oldest-first input. Only a caller comment records it (`analytics_service.py:221`); correct today via `sorted(candidates)`, but a future newest-first caller would silently lose audit rows. Use `min`/`max` in the repository or document the precondition. - **P3 `occupancy_repository.py:472`**: `dict(stored)` raises `TypeError` → 500 if row 1 is missing (the old UPDATE was a no-op). `init_schema` always seeds the row, so theoretical. - **P3 (possible Duplicated Code)**: the seed literals in `database.py:284,300-306` duplicate `DEFAULT_HOLIDAY_HOURS` / `DEFAULT_WEEKDAY_HOURS`. - No standards breach in the fix commit. Ruff clean; full suite 364 passed, 1 skipped. **Merge readiness (this axis):** Ready. P3s → follow-up issue. ## Spec Fix commit `67a37e3` checked against pass-1 head `6f0882f` and #105. The three baseline test files pass (27) in a throwaway worktree; mutation check: restoring the daypart minimum to `< 2` fails `test_hourly_and_daypart_baselines_use_the_same_configured_minimum`. ### Pass-1 findings | Finding | Verdict | Evidence | |---|---|---| | P2 daypart baseline hard-coded minimum 2 | ✅ fixed | `analytics_service.py:965` is `if len(usable) < rules.min_samples:`; summary (`:446`), hourly (`:1216`) and daypart (`:945`, `:965`) all use `rules.min_samples`. Test at `tests/test_statistics_baseline_rules.py:220` (min 1, one usable day → baseline on both routes) fails with the old `2`. | | P2 dwell weighting missing from API docs | ✅ fixed | `docs/api/README.md:128`: "Daypart dwell in the baseline is weighted by each source day's entries (Σ dwell × entries / Σ entries)", matching `analytics_service.py:985-989`; the glossary still leaves it out, as specified. | | P3 unsettled rules (min ≤ max ≤ lookback, 52-week limit, admin fields) | ✅ resolved | Confirmed by the maintainer; merged-config validation now runs inside the write transaction (`occupancy_repository.py:449-483`), covered by `test_concurrent_saves_cannot_store_inconsistent_sampling_rules`. | | P3 #113 should name the call site | ✅ fixed | #113 comment names `ScheduleCalendar.opening_hours(day)`, `ScheduleCalendar.is_closed(day)` and `get_active_schedule_info_async`, and restates the limitation. | ### New findings None at P1/P2. The reworked candidate selection keeps every source-day condition in the glossary (*"A day counts only if it has counted flow, is not a holiday or Closed Day, has no ingestion gap, is trusted in its current audit, and had the same opening hours."*): - Calendar checks (`analytics_service.py:209-219`: holiday, Closed Day, same hours, before the current business day) still run before any database read. - Counted flow and gaps come from one batched `get_counted_cycle_quality_range_async` read (`occupancy_repository.py:1441`): `has_data` (EXISTS on counted-camera events) matches the old `COUNT(*) > 0`; `has_gap` uses the same overlap rule as the old `_cycle_quality_async`; candidates are sorted oldest first for the query's `BETWEEN`. - Trust is still read per candidate, newest first, stopping at `max_samples` — same selected days and order as before. Glossary, the rename to `baseline=usual_weekday` on all four routes and the 4/8/2 defaults are unchanged; the migration now builds its defaults from the named constants. **Merge readiness (this axis):** Ready. --- **Summary:** Standards: no P1/P2; 3 P3 (worst: hidden oldest-first precondition in the batched quality query). Spec: no findings; ready. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg referenced this pull request 2026-09-25 22:11:36 +00:00

follow-up(statistics): P3 cleanups from #122 review (baseline rules) #124

gabogg commented 2026-09-25 22:13:18 +00:00

Author

Owner

Copy link

Pass 2: no P1/P2 on either axis. Remaining P3s filed as #124. Tested merged with current master: 364 passed, 1 skipped. Merging.

🤖 Generated with Claude Code

Pass 2: no P1/P2 on either axis. Remaining P3s filed as #124. Tested merged with current `master`: 364 passed, 1 skipped. Merging. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg merged commit 4bd8158870 into master 2026-09-25 22:13:20 +00:00

gabogg deleted branch feat/statistics-baseline-rules 2026-09-25 22:13:20 +00:00

gabogg referenced this pull request from a commit 2026-09-25 22:13:21 +00:00

feat(statistics): usual weekday baseline matches opening hours; configurable sampling (#105) (#122)

gabogg referenced this pull request 2026-09-25 23:07:50 +00:00

fix(statistics): daypart dwell multiplier, usual daypart labels, per-day excluded flag (#106) #126

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

No description provided.