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

Merged

gabogg merged 2 commits from fix/statistics-dwell-labels-excluded into master 2026-09-26 01:41:35 +00:00

Conversation 3 Commits 2 Files changed 7 +324 -53

gabogg commented 2026-09-25 23:07:50 +00:00

Owner

Copy link

Closes #106

Summary

Step 4 of the pre-deck work (2026-09-25 decisions). This fixes three things the deck would show wrongly:

• Daypart dwell uses the cycle's own calibration. Previously dayparts used today's active multiplier while the summary and daily series used the cycle's, so one day could show two dwell values on the same view.
• Usual dayparts have clock labels.
• Daily rows carry the cycle's audit verdict (excluded and trusted), so the deck's data-quality marker doesn't need a trust-score threshold.

Architectural impact

• One rule for "which multiplier did this past cycle use". Two private helpers in AnalyticsService replace three separate copies:

  • _trusted_cycle_log_async(day, bounds) returns the cycle's calibration log only if it is trusted and its computed_exit_multiplier is above MIN_PLAUSIBLE_EXIT_MULTIPLIER; otherwise it returns None.
  • _cycle_exit_multiplier_async(day, bounds, active) returns that log's multiplier, else the active multiplier.

  The daily series, dayparts (day and week views) and usual-weekday daypart samples use the multiplier helper. The hourly series uses the log helper, because it also takes the guard count from the trusted log.

• MIN_PLAUSIBLE_EXIT_MULTIPLIER = 0.5 (app/schemas/occupancy_models.py) names the cut-off calibration already used when learning k. occupancy_service.py now uses the constant in both of its places. Behaviour change: a trusted cycle multiplier of 0.5 or below (including 0.0) is no longer applied by any statistics view. The view falls back to the active multiplier and the hourly series ignores that log's guard count. Before, the daily series accepted such values and the hourly series ignored only 0.0.

• Contract additions (required fields):

  • UsualDaypart.label: str | None, e.g. 10:00–14:20. It is set when every source day has the same clock span (normal since #105) and null if the spans differ.
  • DailyStatistics.excluded: the current nocturnal audit is AUTO_EXCLUDED, MANUAL_EXCLUDED or ANOMALOUS_QUARANTINE. This is the same rule as the picker's excluded_days.
  • DailyStatistics.trusted: the cycle's highest-priority current, unsuperseded verdict (retroactive audit, then manual, then automatic). No verdict means untrusted.

• Verdict priority in one place. _CYCLE_VERDICT_PRIORITY_SQL in occupancy_repository.py is shared by get_counted_cycle_quality_range_async (new verdicts CTE, is_trusted column) and the trusted-history query, which used to repeat the CASE.

  • Usual-weekday baseline selection reads trust from that batched query. is_cycle_data_trusted_async is removed, so there is no longer one query per candidate day.
  • The range query now accepts cycles in any order (it uses the min and max date).

• API docs updated. No migration.

• Resolves the related items in #97 (duplicated verdict CASE), #99 (one trusted-multiplier helper; dayparts multiplier) and #124 (per-candidate trust reads; range-query ordering). Split out: #127 (the per-cycle log lookup can return a neighbouring cycle's log).

Verification

tests/test_statistics_dwell_labels_excluded.py (8 tests, real ASGI + in-memory SQLite, no mocks):

• dayparts with a trusted cycle multiplier give the same occupancies as running with that multiplier active (mutation-checked against the active multiplier);
• week-view dayparts use each day's own multiplier;
• an implausible cycle multiplier falls back to the active one, and its guard count is not applied;
• the hourly series uses the trusted cycle's multiplier and guard count;
• usual daypart labels equal the target day's labels, and are withheld when source days disagree;
• the daily series flags excluded cycles, and trusted follows the verdict matrix: trusted, auto-excluded, quarantine, pending, no verdict.

pytest: 376 passed, 1 skipped. Frontend: 80/80. Ruff, pre-commit and scripts/check_docs.py passed. Reviewed in two passes (see comments); pass-2 P3s are filed as a follow-up issue.

Checklist

• Daypart dwell uses the cycle's trusted, plausible multiplier, through one shared helper.
• UsualDaypart.label (null when source days disagree).
• DailyStatistics.excluded and DailyStatistics.trusted.
• API docs.

🤖 Generated with Claude Code

Closes #106 ## Summary Step 4 of the pre-deck work (2026-09-25 decisions). This fixes three things the deck would show wrongly: - **Daypart dwell uses the cycle's own calibration.** Previously dayparts used today's active multiplier while the summary and daily series used the cycle's, so one day could show two dwell values on the same view. - **Usual dayparts have clock labels.** - **Daily rows carry the cycle's audit verdict** (`excluded` and `trusted`), so the deck's data-quality marker doesn't need a trust-score threshold. ## Architectural impact - **One rule for "which multiplier did this past cycle use".** Two private helpers in `AnalyticsService` replace three separate copies: - `_trusted_cycle_log_async(day, bounds)` returns the cycle's calibration log only if it is trusted and its `computed_exit_multiplier` is above `MIN_PLAUSIBLE_EXIT_MULTIPLIER`; otherwise it returns `None`. - `_cycle_exit_multiplier_async(day, bounds, active)` returns that log's multiplier, else the active multiplier. The daily series, dayparts (day and week views) and usual-weekday daypart samples use the multiplier helper. The hourly series uses the log helper, because it also takes the guard count from the trusted log. - **`MIN_PLAUSIBLE_EXIT_MULTIPLIER = 0.5`** (`app/schemas/occupancy_models.py`) names the cut-off calibration already used when learning k. `occupancy_service.py` now uses the constant in both of its places. Behaviour change: a trusted cycle multiplier of 0.5 or below (including `0.0`) is no longer applied by any statistics view. The view falls back to the active multiplier and the hourly series ignores that log's guard count. Before, the daily series accepted such values and the hourly series ignored only `0.0`. - **Contract additions (required fields):** - `UsualDaypart.label: str | None`, e.g. `10:00–14:20`. It is set when every source day has the same clock span (normal since #105) and `null` if the spans differ. - `DailyStatistics.excluded`: the current nocturnal audit is `AUTO_EXCLUDED`, `MANUAL_EXCLUDED` or `ANOMALOUS_QUARANTINE`. This is the same rule as the picker's `excluded_days`. - `DailyStatistics.trusted`: the cycle's highest-priority current, unsuperseded verdict (retroactive audit, then manual, then automatic). No verdict means untrusted. - **Verdict priority in one place.** `_CYCLE_VERDICT_PRIORITY_SQL` in `occupancy_repository.py` is shared by `get_counted_cycle_quality_range_async` (new `verdicts` CTE, `is_trusted` column) and the trusted-history query, which used to repeat the CASE. - Usual-weekday baseline selection reads trust from that batched query. `is_cycle_data_trusted_async` is removed, so there is no longer one query per candidate day. - The range query now accepts cycles in any order (it uses the min and max date). - API docs updated. No migration. - Resolves the related items in #97 (duplicated verdict CASE), #99 (one trusted-multiplier helper; dayparts multiplier) and #124 (per-candidate trust reads; range-query ordering). Split out: #127 (the per-cycle log lookup can return a neighbouring cycle's log). ## Verification `tests/test_statistics_dwell_labels_excluded.py` (8 tests, real ASGI + in-memory SQLite, no mocks): - dayparts with a trusted cycle multiplier give the same occupancies as running with that multiplier active (mutation-checked against the active multiplier); - week-view dayparts use each day's own multiplier; - an implausible cycle multiplier falls back to the active one, and its guard count is not applied; - the hourly series uses the trusted cycle's multiplier and guard count; - usual daypart labels equal the target day's labels, and are withheld when source days disagree; - the daily series flags excluded cycles, and `trusted` follows the verdict matrix: trusted, auto-excluded, quarantine, pending, no verdict. `pytest`: 376 passed, 1 skipped. Frontend: 80/80. Ruff, pre-commit and `scripts/check_docs.py` passed. Reviewed in two passes (see comments); pass-2 P3s are filed as a follow-up issue. ## Checklist - [x] Daypart dwell uses the cycle's trusted, plausible multiplier, through one shared helper. - [x] `UsualDaypart.label` (null when source days disagree). - [x] `DailyStatistics.excluded` and `DailyStatistics.trusted`. - [x] API docs. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg added 1 commit 2026-09-25 23:07:51 +00:00

fix(statistics): daypart dwell multiplier, usual daypart labels, per-day excluded flag ... fd129794e2

- One helper, `_cycle_exit_multiplier_async`, picks the exit multiplier a
  past cycle was measured under: its trusted computed multiplier, else the
  active one (ADR 0006). The daily series, hourly series and dayparts all
  use it; dayparts previously used today's active multiplier, so the same
  day could show two dwell values on one deck view. The hourly series now
  accepts a trusted multiplier of 0.0 like the others (`is not None`).
- Each usual daypart carries its clock `label`, taken from the source days,
  which share the target day's opening hours (#105).
- Daily rows carry `excluded` from the cycle's current audit verdict, so the
  deck's data-quality marker can mark the day.

Closes #106

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

gabogg commented 2026-09-25 23:16:50 +00:00

Author

Owner

Copy link

Code review — pass 1 (two-axis)

Reviewed git diff origin/master...origin/fix/statistics-dwell-labels-excluded (fd12979) against #106. 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

Checked at fd12979 in a throwaway worktree: ruff check / ruff format --check clean; pytest 367 passed, 1 skipped. No hard violations of AGENTS.md, code-standards.md or git-and-workflow.md: no inline SQL, the helper is typed with a docstring, tests included, API docs updated.

Bugs

• P2 app/services/analytics_service.py:281: a multiplier of 0.0 is now accepted. Going from truthiness to is not None makes the hourly series use a trusted computed_exit_multiplier of 0, and the guard-count override at :1067 now applies too. k = 0 removes every exit, so occupancy becomes cumulative ingress — nonsense. occupancy_service.py:909 already ignores values ≤ 0.5 when learning k. The helper should reject non-positive values (or values outside OPERATIONAL_GUARDRAIL_MIN/MAX) and fall back to the active multiplier, aligning all three views on the safe side. No test covers the hourly change.
• P2, inherited (:278 → occupancy_repository.py:2171): the log lookup can return another cycle's log. The query matches cycle_date = ? OR timestamp_epoch BETWEEN start AND end and ranks manual types first, so a MANUAL_OVERRIDE for cycle D written during D+1's window beats D+1's own automatic log, and D+1 gets D's k and guard count; same when D's nocturnal audit runs after the reset. Not introduced here, but dayparts now inherit it. (bounds.end vs next_start is consistent: end is inclusive and the repo compares with <=.) Follow-up: key the lookup on cycle_date only.
• P3 :867: seven extra queries for the week view (one per day) — small next to the ~6 per day the daypart loop already makes; could be batched later.
• P3 :989: usable[0][1][index].label is safe today (usable keeps only 3-part days; volume mode with a baseline is rejected). Labels match only because the schedule comes from the current rules (#113); with per-day schedules, source days could differ. Assert, or omit the label on mismatch.

Judgement / smells

• P3 app/schemas/statistics.py:63: excluded: bool = False defaults a field the service always sets; a missing assignment would silently read "not excluded". Make it required.
• P3 :865-866: cycle_start, cycle_end, _ = cycle_bounds unpacks right after binding; use cycle_bounds.start / .end.
• P3 :272, possible Mysterious Name / Middle Man: the helper returns (multiplier, log | None) and two of three callers discard the log. Consider a multiplier-only helper plus a trusted-log accessor for the hourly series.
• Magic numbers: none added (the 1.0 / 8 defaults at :857, :1045 pre-exist).

Tests

Three new real ASGI + SQLite flows, no mocks. Gap: no test for the hourly guard-count override or the 0.0 path.

Spec

Verdict: all three decisions implemented; no scope creep; one partial requirement on the data-quality marker (P2).

(a) Missing or partial

• P2: the "untrusted" verdict is not exposed per day (app/schemas/statistics.py:63, app/db/occupancy_repository.py:1473). Spec: "The data-quality marker is triggered by the audit verdict (excluded/untrusted), gap estimates and missing days — not by a trust-score threshold." Daily rows carry excluded, gap_estimated, has_data (missing days derivable as !has_data && !is_closed) and data_trust (a score) — but no untrusted-but-not-excluded verdict. A PENDING_AUDIT cycle and a cycle with no current nocturnal verdict both return excluded=false; the deck could only catch them with a data_trust threshold, which the spec rules out. The usual-weekday baseline already applies this rule via is_cycle_data_trusted_async (docs: "Days with no current audit verdict or a current untrusted verdict are excluded"). Add trusted: bool from the same verdict (or an audit-status enum) next to excluded, or record in the issue that excluded alone triggers the marker.
• P3: daypart and hourly responses carry no excluded flag (analytics_service.py:945-951); the daypart week view has no per-day quality fields at all. The spec only requires DailyStatistics.excluded, so this is a note for the deck.

(b) Scope creep

None. The hourly series now uses is not None (a trusted 0.0 is no longer ignored) — exactly the drift the shared helper was asked to fix.

(c) Implemented but wrong

None found.

• Multiplier coverage: every past-cycle path goes through _cycle_exit_multiplier_async (analytics_service.py:272-283): daily series (:750), dayparts day and week views (:867, per day), hourly (:1064), usual-weekday daypart samples (via the recursive get_dwell_dayparts_async), the summary's peak and mean dwell (from the daily series), and the multi-day overlay (via hourly). Remaining active_exit_multiplier reads in statistics paths are only the fallback passed into the helper.
• Window drift: the helper uses bounds.end for the log window, as both old copies did.
• Excluded semantics: excluded comes from the same get_counted_cycle_quality_range_async row (is_excluded) as the picker's excluded_days.
• Labels: UsualDaypart.label from usable[0] (:989) is valid since #105 guarantees identical opening hours.
• P3 (existing behaviour now reaching dayparts): get_calibration_log_for_cycle_async also matches any current log whose timestamp_epoch falls in the cycle window and doesn't filter superseded_by; live-day dayparts can pick up the previous cycle's audit if it was stamped after the reset. Follow-up.

Tests

The 3 new tests pass and each fails without its change (swapping the helper back to the active multiplier fails the dayparts test; the label and excluded tests fail on the missing field). Not covered: the hourly 0.0 behaviour change; AUTO_EXCLUDED and ANOMALOUS_QUARANTINE (only MANUAL_EXCLUDED); the week-view daypart path.

---

Summary: Standards 9 (2×P2, 7×P3), worst: a trusted multiplier of 0.0 is now accepted by the hourly series. Spec 5 (1×P2, 4×P3), worst: daily rows don't expose an untrusted-but-not-excluded verdict for the data-quality marker.

🤖 Generated with Claude Code

# Code review — pass 1 (two-axis) Reviewed `git diff origin/master...origin/fix/statistics-dwell-labels-excluded` (fd12979) against #106. 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 Checked at `fd12979` in a throwaway worktree: `ruff check` / `ruff format --check` clean; `pytest` 367 passed, 1 skipped. No hard violations of AGENTS.md, code-standards.md or git-and-workflow.md: no inline SQL, the helper is typed with a docstring, tests included, API docs updated. ### Bugs - **P2 `app/services/analytics_service.py:281`: a multiplier of `0.0` is now accepted.** Going from truthiness to `is not None` makes the hourly series use a trusted `computed_exit_multiplier` of 0, and the guard-count override at `:1067` now applies too. k = 0 removes every exit, so occupancy becomes cumulative ingress — nonsense. `occupancy_service.py:909` already ignores values ≤ 0.5 when learning k. The helper should reject non-positive values (or values outside `OPERATIONAL_GUARDRAIL_MIN/MAX`) and fall back to the active multiplier, aligning all three views on the safe side. No test covers the hourly change. - **P2, inherited (`:278` → `occupancy_repository.py:2171`): the log lookup can return another cycle's log.** The query matches `cycle_date = ? OR timestamp_epoch BETWEEN start AND end` and ranks manual types first, so a `MANUAL_OVERRIDE` for cycle D written during D+1's window beats D+1's own automatic log, and D+1 gets D's k and guard count; same when D's nocturnal audit runs after the reset. Not introduced here, but dayparts now inherit it. (`bounds.end` vs `next_start` is consistent: `end` is inclusive and the repo compares with `<=`.) Follow-up: key the lookup on `cycle_date` only. - **P3 `:867`: seven extra queries for the week view** (one per day) — small next to the ~6 per day the daypart loop already makes; could be batched later. - **P3 `:989`: `usable[0][1][index].label` is safe today** (`usable` keeps only 3-part days; volume mode with a baseline is rejected). Labels match only because the schedule comes from the current rules (#113); with per-day schedules, source days could differ. Assert, or omit the label on mismatch. ### Judgement / smells - **P3 `app/schemas/statistics.py:63`**: `excluded: bool = False` defaults a field the service always sets; a missing assignment would silently read "not excluded". Make it required. - **P3 `:865-866`**: `cycle_start, cycle_end, _ = cycle_bounds` unpacks right after binding; use `cycle_bounds.start` / `.end`. - **P3 `:272`, possible Mysterious Name / Middle Man**: the helper returns `(multiplier, log | None)` and two of three callers discard the log. Consider a multiplier-only helper plus a trusted-log accessor for the hourly series. - **Magic numbers:** none added (the `1.0` / `8` defaults at `:857`, `:1045` pre-exist). ### Tests Three new real ASGI + SQLite flows, no mocks. Gap: no test for the hourly guard-count override or the `0.0` path. ## Spec **Verdict:** all three decisions implemented; no scope creep; one partial requirement on the data-quality marker (P2). ### (a) Missing or partial - **P2: the "untrusted" verdict is not exposed per day** (`app/schemas/statistics.py:63`, `app/db/occupancy_repository.py:1473`). Spec: *"The data-quality marker is triggered by the audit verdict (excluded/untrusted), gap estimates and missing days — not by a trust-score threshold."* Daily rows carry `excluded`, `gap_estimated`, `has_data` (missing days derivable as `!has_data && !is_closed`) and `data_trust` (a score) — but no untrusted-but-not-excluded verdict. A `PENDING_AUDIT` cycle and a cycle with no current nocturnal verdict both return `excluded=false`; the deck could only catch them with a `data_trust` threshold, which the spec rules out. The usual-weekday baseline already applies this rule via `is_cycle_data_trusted_async` (docs: "Days with no current audit verdict or a current untrusted verdict are excluded"). Add `trusted: bool` from the same verdict (or an audit-status enum) next to `excluded`, or record in the issue that `excluded` alone triggers the marker. - **P3: daypart and hourly responses carry no `excluded` flag** (`analytics_service.py:945-951`); the daypart week view has no per-day quality fields at all. The spec only requires `DailyStatistics.excluded`, so this is a note for the deck. ### (b) Scope creep None. The hourly series now uses `is not None` (a trusted `0.0` is no longer ignored) — exactly the drift the shared helper was asked to fix. ### (c) Implemented but wrong None found. - **Multiplier coverage:** every past-cycle path goes through `_cycle_exit_multiplier_async` (`analytics_service.py:272-283`): daily series (`:750`), dayparts day and week views (`:867`, per day), hourly (`:1064`), usual-weekday daypart samples (via the recursive `get_dwell_dayparts_async`), the summary's peak and mean dwell (from the daily series), and the multi-day overlay (via hourly). Remaining `active_exit_multiplier` reads in statistics paths are only the fallback passed into the helper. - **Window drift:** the helper uses `bounds.end` for the log window, as both old copies did. - **Excluded semantics:** `excluded` comes from the same `get_counted_cycle_quality_range_async` row (`is_excluded`) as the picker's `excluded_days`. - **Labels:** `UsualDaypart.label` from `usable[0]` (`:989`) is valid since #105 guarantees identical opening hours. - **P3 (existing behaviour now reaching dayparts):** `get_calibration_log_for_cycle_async` also matches any current log whose `timestamp_epoch` falls in the cycle window and doesn't filter `superseded_by`; live-day dayparts can pick up the previous cycle's audit if it was stamped after the reset. Follow-up. ### Tests The 3 new tests pass and each fails without its change (swapping the helper back to the active multiplier fails the dayparts test; the label and `excluded` tests fail on the missing field). Not covered: the hourly `0.0` behaviour change; `AUTO_EXCLUDED` and `ANOMALOUS_QUARANTINE` (only `MANUAL_EXCLUDED`); the week-view daypart path. --- **Summary:** Standards 9 (2×P2, 7×P3), worst: a trusted multiplier of 0.0 is now accepted by the hourly series. Spec 5 (1×P2, 4×P3), worst: daily rows don't expose an untrusted-but-not-excluded verdict for the data-quality marker. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg referenced this pull request 2026-09-25 23:37:19 +00:00

fix(calibration): per-cycle calibration log lookup can return a neighbouring cycle's log #127

gabogg commented 2026-09-25 23:43:46 +00:00

Author

Owner

Copy link

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

Standards

Finding	Fix
P2 multiplier 0.0 accepted	New MIN_PLAUSIBLE_EXIT_MULTIPLIER (the 0.5 calibration already used, now named and used in both of its places); _trusted_cycle_log_async only returns a trusted log whose multiplier is above it, so a 0.0 falls back to the active k in all three views. Tests: implausible multiplier falls back (and doesn't apply the guard override); a plausible one sets k and guards. Mutation-checked.
P2 lookup can return a neighbouring cycle's log (inherited)	Split out as #127 (maintainer decision).
P3 seven extra queries in the week view	Kept (small next to the loop's existing queries); a week-view test now pins per-day multipliers.
P3 usable[0] label assumption	_shared_label returns the label only if every source day agrees, else null (UsualDaypart.label: str | None); unit-tested.
P3 excluded: bool = False default	excluded and the new trusted are required.
P3 redundant unpacking	cycle_bounds.start / .end.
P3 helper returns an ignored log	Split into _trusted_cycle_log_async (hourly needs the log for guards) and _cycle_exit_multiplier_async (multiplier only).
Tests: hourly guard override / 0.0 path	Added.

Spec

Finding	Fix
P2 no untrusted-but-not-excluded verdict per day	DailyStatistics.trusted from the cycle's highest-priority current verdict (retroactive > manual > automatic; no verdict = untrusted), the same rule the usual-weekday baseline uses. It's computed in the batched get_counted_cycle_quality_range_async with one shared _CYCLE_VERDICT_PRIORITY_SQL fragment, which also replaced the duplicated CASE in the trusted-history query. Baseline selection now reads it from the batch, so the per-candidate trust query (and is_cycle_data_trusted_async) is gone. Test matrix: trusted, auto-excluded, quarantine, pending, no verdict.
P3 dayparts/hourly carry no excluded	Noted for the deck (not in #106); the deck reads per-day quality from the daily series.
P3 lookup ignores superseded_by / matches neighbouring logs	Part of #127.
Tests: AUTO_EXCLUDED, ANOMALOUS_QUARANTINE, week view	Added.

Side effects noted for the follow-up issues: #124 (per-candidate trust reads; range-query ordering precondition) and #97 (duplicated verdict-priority CASE) are resolved by this commit; #99 (one trusted-multiplier helper; dayparts multiplier) is resolved by this PR.

🤖 Generated with Claude Code

Pass-1 findings addressed in the latest commit. Full suite 376 passed, 1 skipped; frontend 80/80; ruff and docs check clean. ### Standards | Finding | Fix | |---|---| | P2 multiplier `0.0` accepted | New `MIN_PLAUSIBLE_EXIT_MULTIPLIER` (the 0.5 calibration already used, now named and used in both of its places); `_trusted_cycle_log_async` only returns a trusted log whose multiplier is above it, so a `0.0` falls back to the active k in all three views. Tests: implausible multiplier falls back (and doesn't apply the guard override); a plausible one sets k and guards. Mutation-checked. | | P2 lookup can return a neighbouring cycle's log (inherited) | Split out as **#127** (maintainer decision). | | P3 seven extra queries in the week view | Kept (small next to the loop's existing queries); a week-view test now pins per-day multipliers. | | P3 `usable[0]` label assumption | `_shared_label` returns the label only if every source day agrees, else `null` (`UsualDaypart.label: str \| None`); unit-tested. | | P3 `excluded: bool = False` default | `excluded` and the new `trusted` are required. | | P3 redundant unpacking | `cycle_bounds.start` / `.end`. | | P3 helper returns an ignored log | Split into `_trusted_cycle_log_async` (hourly needs the log for guards) and `_cycle_exit_multiplier_async` (multiplier only). | | Tests: hourly guard override / `0.0` path | Added. | ### Spec | Finding | Fix | |---|---| | P2 no untrusted-but-not-excluded verdict per day | `DailyStatistics.trusted` from the cycle's highest-priority current verdict (retroactive > manual > automatic; no verdict = untrusted), the same rule the usual-weekday baseline uses. It's computed in the batched `get_counted_cycle_quality_range_async` with one shared `_CYCLE_VERDICT_PRIORITY_SQL` fragment, which also replaced the duplicated CASE in the trusted-history query. Baseline selection now reads it from the batch, so the per-candidate trust query (and `is_cycle_data_trusted_async`) is gone. Test matrix: trusted, auto-excluded, quarantine, pending, no verdict. | | P3 dayparts/hourly carry no `excluded` | Noted for the deck (not in #106); the deck reads per-day quality from the daily series. | | P3 lookup ignores `superseded_by` / matches neighbouring logs | Part of **#127**. | | Tests: `AUTO_EXCLUDED`, `ANOMALOUS_QUARANTINE`, week view | Added. | Side effects noted for the follow-up issues: #124 (per-candidate trust reads; range-query ordering precondition) and #97 (duplicated verdict-priority CASE) are resolved by this commit; #99 (one trusted-multiplier helper; dayparts multiplier) is resolved by this PR. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg added 1 commit 2026-09-25 23:43:48 +00:00

fix(statistics): address review of the cycle multiplier and quality flags ... 57fc02af38

- Only a plausible cycle multiplier (above MIN_PLAUSIBLE_EXIT_MULTIPLIER,
  the 0.5 calibration already used and now names) is applied; a trusted
  0.0 falls back to the active multiplier instead of dropping every exit.
- The helper is split: `_trusted_cycle_log_async` (used by the hourly
  series for its guard count) and `_cycle_exit_multiplier_async` returning
  the multiplier only.
- Daily rows carry `trusted` from the cycle's highest-priority current
  verdict, read in the same batched quality query as `excluded`; the verdict
  priority is one shared SQL fragment, and baseline selection now uses the
  batched verdict instead of one query per candidate (the per-candidate
  method is removed). The range query no longer depends on input order.
- `excluded` and `trusted` are required fields; the usual daypart label is
  withheld when source days disagree.
- Tests: verdict matrix (trusted, auto/manual excluded, quarantine, pending,
  no verdict), implausible multiplier fallback, hourly guard override, week
  view per-day multiplier, label disagreement.

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

gabogg commented 2026-09-26 01:39:09 +00:00

Author

Owner

Copy link

Code review — pass 2 (two-axis)

Re-reviewed at 57fc02a against #106. Each axis checked every pass-1 finding against the code (not the resolution text), then reviewed the fix commit for new problems. Verdicts: ✅ fixed · ◐ partial · ❌ not addressed · ↺ regressed · ⊘ declined or split out with a reason. Per policy, P1/P2 block merge; P3s go to one follow-up issue.

Standards

Checked in a throwaway worktree: ruff check / ruff format --check clean; pytest 376 passed, 1 skipped.

Pass-1 findings

Finding	Verdict	Evidence
P2 multiplier 0.0 accepted	✅	analytics_service.py:279-301 requires > MIN_PLAUSIBLE_EXIT_MULTIPLIER (occupancy_models.py:201, reused at occupancy_service.py:910,1172); the hourly guard override follows the plausible log (:1083-1088).
P2 lookup can return a neighbouring cycle's log	⊘	Split out as #127.
P3 seven extra queries in the week view	⊘	Kept with a reason; a per-day week-view test was added.
P3 usable[0] label assumption	✅	_shared_label (:116); UsualDaypart.label: str | None.
P3 excluded: bool = False default	✅	excluded and trusted are required (statistics.py:63-64).
P3 redundant unpacking	✅	:885.
P3 helper returns an ignored log	✅	Split into _trusted_cycle_log_async / _cycle_exit_multiplier_async.
Tests: hourly guard / 0.0 path	✅	Added.

New findings

• P2, git-and-workflow.md §2.2: the PR description is out of date. "Architectural impact" still says the helper returns (multiplier, log) and that "a trusted multiplier of 0.0 is no longer ignored"; the code now does the opposite. It doesn't mention DailyStatistics.trusted, MIN_PLAUSIBLE_EXIT_MULTIPLIER, _CYCLE_VERDICT_PRIORITY_SQL or the removed is_cycle_data_trusted_async, and Verification still says 3 tests / 367 passed.
• P3 stale comment: analytics_service.py:230-231 says "oldest first, as the range query expects; the trust verdict is then read only until enough days pass". Neither is true now, and the sorted(candidates) is no longer needed.
• P3 stale docs: docs/api/README.md:128 says the label "is the same for all of them"; it can now be null.
• P3 typing (code-standards §2.2/§2.3): the test helper occupancies(response) has no annotations; _shared_label takes list[Any] instead of the daypart model type.
• P3 Duplicated Code: the test copies the event-seeding loop from seed_day; extract a seed_events(day) helper.
• P3 test gaps (mutation-checked): changing the threshold to > 0 still passes (nothing pins the 0.5 boundary), and reverting to usable[0][1][index].label still passes (the helper's wiring into the response isn't tested). Caught: dropping the plausibility guard, deriving trusted from excluded, and the baseline ignoring trust.
• P3 judgement: excluded and trusted come from different verdict rules; see Spec.

Merge readiness (this axis): ready once the PR description is updated.

Spec

pytest 376 passed, 1 skipped.

Pass-1 findings

Finding	Verdict	Evidence
P2 no untrusted-but-not-excluded flag per day	✅	DailyStatistics.trusted is required, filled from the verdicts CTE (occupancy_repository.py:1462-1470,1494-1495), and a missing verdict counts as untrusted; test matrix at tests/test_statistics_dwell_labels_excluded.py:161.
P3 dayparts/hourly carry no excluded	⊘	Outside #106; the deck reads per-day quality from the daily series.
P3 lookup ignores superseded_by / matches a neighbouring cycle	⊘	Split out as #127; get_calibration_log_for_cycle_async is unchanged.

Baseline equivalence: the batch reproduces the old is_cycle_data_trusted_async rule exactly: same filter (is_current=1 AND superseded_by IS NULL), same priority order and tie-breaks, and no verdict counts as untrusted. The only change is dropping the early stop, which affects speed only. The MIN_PLAUSIBLE_EXIT_MULTIPLIER fallback also applies to the daily series; that is justified by the pass-1 P2 and documented.

New findings

• P3 (c) excluded and trusted can disagree. Spec: "The data-quality marker is triggered by the audit verdict (excluded/untrusted)". is_excluded reads the latest AUTOMATIC_NOCTURNAL log (occupancy_repository.py:1471-1480), while is_trusted reads the highest-priority verdict. Supersession only runs between nocturnal logs, so an AUTO_EXCLUDED day that an admin overrides reads excluded=true, trusted=true, and the baseline would use it. Either derive excluded from the same ranked verdict or document which flag wins. This fits with #127.
• P3 (a) the docs don't say that a label can be null (docs/api/README.md:128). Spec: "each UsualDaypart carries its start/end clock labels". Returning null is a reasonable safeguard, but the contract should say so.
• P3 the stale comment at analytics_service.py:231 (same as Standards).
• Note, no action: the spec names get_calibration_audit, which doesn't exist; the drift it describes was between the daily-series and hourly lookups, and both now go through _trusted_cycle_log_async.

No scope creep. Merge readiness (this axis): ready.

---

Summary: Standards: 1 P2 + 7 P3; worst is the out-of-date PR description. Spec: 3 P3; worst is that excluded and trusted can disagree. After the description is updated, the P3s go to one follow-up issue and the PR can merge.

🤖 Generated with Claude Code

# Code review — pass 2 (two-axis) Re-reviewed at `57fc02a` against #106. Each axis checked every pass-1 finding against the code (not the resolution text), then reviewed the fix commit for new problems. Verdicts: ✅ fixed · ◐ partial · ❌ not addressed · ↺ regressed · ⊘ declined or split out with a reason. Per policy, P1/P2 block merge; P3s go to one follow-up issue. ## Standards Checked in a throwaway worktree: `ruff check` / `ruff format --check` clean; `pytest` 376 passed, 1 skipped. ### Pass-1 findings | Finding | Verdict | Evidence | |---|---|---| | P2 multiplier `0.0` accepted | ✅ | `analytics_service.py:279-301` requires `> MIN_PLAUSIBLE_EXIT_MULTIPLIER` (`occupancy_models.py:201`, reused at `occupancy_service.py:910,1172`); the hourly guard override follows the plausible log (`:1083-1088`). | | P2 lookup can return a neighbouring cycle's log | ⊘ | Split out as #127. | | P3 seven extra queries in the week view | ⊘ | Kept with a reason; a per-day week-view test was added. | | P3 `usable[0]` label assumption | ✅ | `_shared_label` (`:116`); `UsualDaypart.label: str \| None`. | | P3 `excluded: bool = False` default | ✅ | `excluded` and `trusted` are required (`statistics.py:63-64`). | | P3 redundant unpacking | ✅ | `:885`. | | P3 helper returns an ignored log | ✅ | Split into `_trusted_cycle_log_async` / `_cycle_exit_multiplier_async`. | | Tests: hourly guard / `0.0` path | ✅ | Added. | ### New findings - **P2, git-and-workflow.md §2.2: the PR description is out of date.** "Architectural impact" still says the helper returns `(multiplier, log)` and that "a trusted multiplier of `0.0` is no longer ignored"; the code now does the opposite. It doesn't mention `DailyStatistics.trusted`, `MIN_PLAUSIBLE_EXIT_MULTIPLIER`, `_CYCLE_VERDICT_PRIORITY_SQL` or the removed `is_cycle_data_trusted_async`, and Verification still says 3 tests / 367 passed. - **P3 stale comment:** `analytics_service.py:230-231` says "oldest first, as the range query expects; the trust verdict is then read only until enough days pass". Neither is true now, and the `sorted(candidates)` is no longer needed. - **P3 stale docs:** `docs/api/README.md:128` says the label "is the same for all of them"; it can now be `null`. - **P3 typing (code-standards §2.2/§2.3):** the test helper `occupancies(response)` has no annotations; `_shared_label` takes `list[Any]` instead of the daypart model type. - **P3 Duplicated Code:** the test copies the event-seeding loop from `seed_day`; extract a `seed_events(day)` helper. - **P3 test gaps (mutation-checked):** changing the threshold to `> 0` still passes (nothing pins the 0.5 boundary), and reverting to `usable[0][1][index].label` still passes (the helper's wiring into the response isn't tested). Caught: dropping the plausibility guard, deriving `trusted` from `excluded`, and the baseline ignoring trust. - **P3 judgement:** `excluded` and `trusted` come from different verdict rules; see Spec. **Merge readiness (this axis):** ready once the PR description is updated. ## Spec `pytest` 376 passed, 1 skipped. ### Pass-1 findings | Finding | Verdict | Evidence | |---|---|---| | P2 no untrusted-but-not-excluded flag per day | ✅ | `DailyStatistics.trusted` is required, filled from the `verdicts` CTE (`occupancy_repository.py:1462-1470,1494-1495`), and a missing verdict counts as untrusted; test matrix at `tests/test_statistics_dwell_labels_excluded.py:161`. | | P3 dayparts/hourly carry no `excluded` | ⊘ | Outside #106; the deck reads per-day quality from the daily series. | | P3 lookup ignores `superseded_by` / matches a neighbouring cycle | ⊘ | Split out as #127; `get_calibration_log_for_cycle_async` is unchanged. | **Baseline equivalence:** the batch reproduces the old `is_cycle_data_trusted_async` rule exactly: same filter (`is_current=1 AND superseded_by IS NULL`), same priority order and tie-breaks, and no verdict counts as untrusted. The only change is dropping the early stop, which affects speed only. The `MIN_PLAUSIBLE_EXIT_MULTIPLIER` fallback also applies to the daily series; that is justified by the pass-1 P2 and documented. ### New findings - **P3 (c) `excluded` and `trusted` can disagree.** Spec: *"The data-quality marker is triggered by the audit verdict (excluded/untrusted)"*. `is_excluded` reads the latest `AUTOMATIC_NOCTURNAL` log (`occupancy_repository.py:1471-1480`), while `is_trusted` reads the highest-priority verdict. Supersession only runs between nocturnal logs, so an `AUTO_EXCLUDED` day that an admin overrides reads `excluded=true, trusted=true`, and the baseline would use it. Either derive `excluded` from the same ranked verdict or document which flag wins. This fits with #127. - **P3 (a) the docs don't say that a label can be `null`** (`docs/api/README.md:128`). Spec: *"each `UsualDaypart` carries its start/end clock labels"*. Returning null is a reasonable safeguard, but the contract should say so. - **P3 the stale comment** at `analytics_service.py:231` (same as Standards). - **Note, no action:** the spec names `get_calibration_audit`, which doesn't exist; the drift it describes was between the daily-series and hourly lookups, and both now go through `_trusted_cycle_log_async`. No scope creep. **Merge readiness (this axis):** ready. --- **Summary:** Standards: 1 P2 + 7 P3; worst is the out-of-date PR description. Spec: 3 P3; worst is that `excluded` and `trusted` can disagree. After the description is updated, the P3s go to one follow-up issue and the PR can merge. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg referenced this pull request 2026-09-26 01:41:04 +00:00

follow-up(statistics): P3 cleanups from #126 review (dwell multiplier, labels, excluded/trusted flags) #128

gabogg merged commit 6209d08cfc into master 2026-09-26 01:41:35 +00:00

gabogg deleted branch fix/statistics-dwell-labels-excluded 2026-09-26 01:41:36 +00:00

gabogg referenced this pull request from a commit 2026-09-26 01:41:36 +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!126

No description provided.