gabogg/hikcentral
Watch
1
0
Fork
You've already forked hikcentral
 0
Code Issues 113 Pull requests 8 Projects Releases Packages Wiki Activity Actions

feat(statistics): compare partial periods by daily average above a coverage threshold (#104) #116

Merged
gabogg merged 3 commits from feat/statistics-comparison-coverage into master 2026-09-25 19:17:14 +00:00
Conversation 4 Commits 3 Files changed 9 +351 -39
gabogg commented 2026-09-25 18:36:35 +00:00
Owner
Copy link

Closes #104

Stacked on #115 (config save fix), which this PR needs: the new setting lives in the same config form, and before #115 the form could not round-trip it. Review #115 first; retarget to master once it merges.

Summary

Partial weeks and months no longer blank every comparison they touch. Complete periods compare totals; if either side is partial and both cover at least the configured share of their business days, they compare daily averages over the covered days; below that the change is withheld (PARTIAL_PERIOD). Step 2 of the pre-deck work agreed on 2026-09-25; resolves the problem behind #100.

Problem

Production has one full-day outage (Sun 13 Sep 2026). Under the previous rule it hid "vs previous week" for two weeks and "vs previous month" for September and all of October.

Architectural impact

  • New occupancy config key statistics_min_comparison_coverage (REAL, default 0.80 as DEFAULT_MIN_COMPARISON_COVERAGE, validated 0.5–1.0), added by the existing in-place column migration, written by update_config_*, editable in the admin config form ("Statistics comparisons").
  • PeriodQuality.coverage (covered / business days; Closed Days excluded per #103).
  • compare_visitors(...) in analytics_service.py is the single rule for the summary comparisons and both entrance comparisons (replaces three inline copies).
  • Contract additions on VisitorComparison: basis (TOTAL | DAILY_AVERAGE) and reference_daily_average. reference_visitors stays the raw reference total.
  • API docs updated. No glossary change (Partial Period keeps its meaning).

Verification

  • New tests/test_statistics_comparison_coverage.py (5): complete periods → TOTAL; a reference week missing one day (6/7) → DAILY_AVERAGE on the summary and the entrance comparison, with the expected change; raising the threshold to 0.90 through the config API withholds it (PARTIAL_PERIOD); thresholds 0.4 and 1.1 are rejected with 422 VALIDATION_ERROR.
  • Existing statistics tests unchanged and passing (the month test's low-coverage case still returns PARTIAL_PERIOD).
  • pytest: 340 passed. Ruff, pre-commit and scripts/check_docs.py passed.

Checklist

  • Config key with named default and 0.5–1.0 validation, editable in the admin.
  • Complete → totals; partial ≥ threshold → daily averages with basis; below → PARTIAL_PERIOD.
  • Summary and entrance comparisons share one rule.
  • API docs.

🤖 Generated with Claude Code

Closes #104 Stacked on #115 (config save fix), which this PR needs: the new setting lives in the same config form, and before #115 the form could not round-trip it. Review #115 first; retarget to `master` once it merges. ## Summary Partial weeks and months no longer blank every comparison they touch. Complete periods compare totals; if either side is partial and both cover at least the configured share of their business days, they compare **daily averages** over the covered days; below that the change is withheld (`PARTIAL_PERIOD`). Step 2 of the pre-deck work agreed on 2026-09-25; resolves the problem behind #100. ## Problem Production has one full-day outage (Sun 13 Sep 2026). Under the previous rule it hid "vs previous week" for two weeks and "vs previous month" for September and all of October. ## Architectural impact - New occupancy config key `statistics_min_comparison_coverage` (`REAL`, default 0.80 as `DEFAULT_MIN_COMPARISON_COVERAGE`, validated 0.5–1.0), added by the existing in-place column migration, written by `update_config_*`, editable in the admin config form ("Statistics comparisons"). - `PeriodQuality.coverage` (covered / business days; Closed Days excluded per #103). - `compare_visitors(...)` in `analytics_service.py` is the single rule for the summary comparisons and both entrance comparisons (replaces three inline copies). - Contract additions on `VisitorComparison`: `basis` (`TOTAL` | `DAILY_AVERAGE`) and `reference_daily_average`. `reference_visitors` stays the raw reference total. - API docs updated. No glossary change (Partial Period keeps its meaning). ## Verification - New `tests/test_statistics_comparison_coverage.py` (5): complete periods → `TOTAL`; a reference week missing one day (6/7) → `DAILY_AVERAGE` on the summary and the entrance comparison, with the expected change; raising the threshold to 0.90 through the config API withholds it (`PARTIAL_PERIOD`); thresholds 0.4 and 1.1 are rejected with `422 VALIDATION_ERROR`. - Existing statistics tests unchanged and passing (the month test's low-coverage case still returns `PARTIAL_PERIOD`). - `pytest`: 340 passed. Ruff, pre-commit and `scripts/check_docs.py` passed. ## Checklist - [x] Config key with named default and 0.5–1.0 validation, editable in the admin. - [x] Complete → totals; partial ≥ threshold → daily averages with `basis`; below → `PARTIAL_PERIOD`. - [x] Summary and entrance comparisons share one rule. - [x] API docs. 🤖 Generated with [Claude Code](https://claude.com/claude-code) 
A partial week or month no longer blanks every comparison it touches.
Complete periods still compare totals. When either side is partial and
both cover at least `statistics_min_comparison_coverage` of their business
days (new occupancy config key, default 0.80, validated to 0.5–1.0), the
comparison uses daily averages over the covered days and says so with
`basis: DAILY_AVERAGE` and `reference_daily_average`. Below the threshold
the change is still withheld with PARTIAL_PERIOD.

One rule (`compare_visitors`) now serves the summary and both entrance
comparisons, replacing three inline copies. The threshold is editable in
the admin config form.

Closes #104

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gabogg commented 2026-09-25 18:43:18 +00:00
Author
Owner
Copy link

Code review — pass 1 (two-axis)

Reviewed the PR's own diff git diff origin/fix/occupancy-config-save-preserves-calibration...origin/feat/statistics-comparison-coverage (7cf4a0b) against #104. 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

Ruff check/format clean; 33 statistics tests pass (throwaway worktree).

Hard violations (a repo convention exists)

  • P2 app/schemas/occupancy_models.py:~331: bounds are bare literals ge=0.5, le=1.0, while the same file names bounds as constants (OPERATIONAL_GUARDRAIL_MIN/MAX, :309-310). Add named floor/ceiling constants. The same numbers are copied as literals in index.html:803 (min="0.5" max="1"), the label text (0.5–1) and docs/api/README.md:134 — four places to change.
  • P3 app/static/index.html:803 value="0.80" and app/db/database.py:247 DEFAULT 0.80 repeat DEFAULT_MIN_COMPARISON_COVERAGE. Consistent with the existing trust_* rows, but still a hard-coded copy.

Real bugs and risks

  • P2 analytics_service.py:122-131: when current_quality is None, now = float(current) is a total; if the reference is partial and above the threshold, before is a daily average — different units. The only None caller today is the day comparison (:375), where a reference day with data always has covered=1, so it's latent. Make current_quality required, or treat None as complete with covered_days from the reference.
  • P3 analytics_service.py:129-130: division by covered_days is only protected by min_coverage > 0, guaranteed by the Pydantic schema; _min_comparison_coverage (:218) reads the raw DB value unclamped, so a stored 0 raises ZeroDivisionError. Clamp or guard and covered_days.
  • Migration: safe — ALTER ... ADD COLUMN with NOT NULL DEFAULT backfills.
  • Config write path (occupancy_repository.py:~363/~450): COALESCE(?, col) keeps the stored value when the key is missing; correct, matches trust_*.
  • Behaviour change, OK: removing the granularity != "day" guard is harmless (current days with covered=0 are rejected at :214; reference days without data return earlier).

Baseline smells (judgement calls)

  • Data Clumps: compare_visitors(reference_period, current, reference, current_quality, reference_quality, min_coverage) — (count, quality) pairs travel together; a small PeriodCount(visitors, quality) removes the None branches.
  • Repeated Switches (:126-129): if current_quality else 1.0 and if current_quality else float(current) repeat the same None check.
  • Middle Man (mild): _min_comparison_coverage (:218) is a one-line wrapper around cfg.get.
  • Primitive Obsession (mild): basis, now, before = "TOTAL", ... infers basis as str, not ComparisonBasis.

UI

  • P3 index.html:799-803: new copy ("STATISTICS COMPARISONS", "MIN COVERAGE…") has no data-i18n (159 uses elsewhere in index.html); neighbouring admin config labels aren't translated either, so consistent with the form — but hard-coded English with bounds in the label.

Spec note

docs/api/README.md says the rule covers "a week or month"; the code applies it to days too. Equivalent outcome, but the docs should say days are never partial.

Spec

Verdict: no P1/P2. Every decision in #104 is in the code; nothing goes against the spec.

(a) Missing or partial

None.

  • "New config key … default 0.80, validated to 0.5–1.0 … editable": column migration app/db/database.py:247, named constant and Field in app/schemas/occupancy_models.py, admin input index.html:796.
  • "Coverage = covered business days / business days (Closed Days excluded)": PeriodQuality.coverage (app/schemas/statistics.py:36); the denominator already excludes Closed Days.
  • "Both complete → totals / ≥ threshold → daily averages … basis / below → PARTIAL_PERIOD": compare_visitors (analytics_service.py:108-143), using >=, so a period exactly at the threshold is compared.
  • "Applies to the summary comparisons and the entrance comparisons": covers previous, last_year, same_weekday_last_week, and entrance previous / last_year.

(b) Scope creep

  • P3: reference_daily_average (statistics.py:129) is a contract addition; harmless. For months it repeats previous_month_daily_average (analytics_service.py:~418), which still divides by covered days whatever the coverage — two sources for the same number. Note it in the docs or retire one later.

(c) Implemented but wrong

Nothing confirmed.

  • P3, analytics_service.py:258: the granularity != "day" exemption is gone. A day is covered (1/1) or not (0/1), and a reference day with no data exits earlier as NO_DATA_*, so nothing changes — unless counted-flow has_data and cycle-quality has_data can disagree for a day. No test pins the day path under the new rule.
  • Checked OK: a partial side with covered_days 0 has coverage 0 → PARTIAL_PERIOD, no division; a fully closed current period returns early; last_year omission unchanged (PARTIAL_PERIOD still carries reference_visitors); usual_weekday correctly out of scope.

Tests

The 5 new tests fail without the change (basis didn't exist; the old rule gave PARTIAL_PERIOD for 6/7). 33 statistics tests pass in a throwaway worktree. An ad-hoc probe with the current week partial (6/7) vs a complete reference gave DAILY_AVERAGE, −16.7% on summary and entrances — correct, but not in the suite.

Gaps (P3, tests/test_statistics_comparison_coverage.py):

  1. No test with the current side partial (only the reference side).
  2. No month test, and no last_year with DAILY_AVERAGE.
  3. No test combining Closed Days with coverage (e.g. 1 Closed Day + 1 missing day → 5/6 → DAILY_AVERAGE).
  4. No test pinning the exact threshold (e.g. 4/5 at 0.80) to lock in >=.
  5. The stricter-threshold case asserts only on the summary, not the entrance comparison.

Minor

Spec: "the partial badge shows". quality.partial stays true; no statistics UI renders it yet, so only the consumer can check it.

Summary: Standards 11 (2×P2, 9×P3), worst: compare_visitors can compare a total against a daily average when current_quality is None (latent today), plus bare 0.5/1.0 bounds copied in four places. Spec 8 (all P3), worst: no test with the current side partial or with Closed Days + coverage.

🤖 Generated with Claude Code

# Code review — pass 1 (two-axis) Reviewed the PR's own diff `git diff origin/fix/occupancy-config-save-preserves-calibration...origin/feat/statistics-comparison-coverage` (7cf4a0b) against #104. 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 Ruff check/format clean; 33 statistics tests pass (throwaway worktree). ### Hard violations (a repo convention exists) - **P2 `app/schemas/occupancy_models.py:~331`**: bounds are bare literals `ge=0.5, le=1.0`, while the same file names bounds as constants (`OPERATIONAL_GUARDRAIL_MIN/MAX`, :309-310). Add named floor/ceiling constants. The same numbers are copied as literals in `index.html:803` (`min="0.5" max="1"`), the label text `(0.5–1)` and `docs/api/README.md:134` — four places to change. - **P3 `app/static/index.html:803`** `value="0.80"` and `app/db/database.py:247` `DEFAULT 0.80` repeat `DEFAULT_MIN_COMPARISON_COVERAGE`. Consistent with the existing `trust_*` rows, but still a hard-coded copy. ### Real bugs and risks - **P2 `analytics_service.py:122-131`**: when `current_quality is None`, `now = float(current)` is a **total**; if the reference is partial and above the threshold, `before` is a **daily average** — different units. The only `None` caller today is the day comparison (`:375`), where a reference day with data always has `covered=1`, so it's latent. Make `current_quality` required, or treat `None` as complete with `covered_days` from the reference. - **P3 `analytics_service.py:129-130`**: division by `covered_days` is only protected by `min_coverage > 0`, guaranteed by the Pydantic schema; `_min_comparison_coverage` (`:218`) reads the raw DB value unclamped, so a stored 0 raises `ZeroDivisionError`. Clamp or guard `and covered_days`. - **Migration**: safe — `ALTER ... ADD COLUMN` with `NOT NULL DEFAULT` backfills. - **Config write path** (`occupancy_repository.py:~363/~450`): `COALESCE(?, col)` keeps the stored value when the key is missing; correct, matches `trust_*`. - **Behaviour change, OK**: removing the `granularity != "day"` guard is harmless (current days with `covered=0` are rejected at `:214`; reference days without data return earlier). ### Baseline smells (judgement calls) - **Data Clumps**: `compare_visitors(reference_period, current, reference, current_quality, reference_quality, min_coverage)` — (count, quality) pairs travel together; a small `PeriodCount(visitors, quality)` removes the `None` branches. - **Repeated Switches** (`:126-129`): `if current_quality else 1.0` and `if current_quality else float(current)` repeat the same `None` check. - **Middle Man (mild)**: `_min_comparison_coverage` (`:218`) is a one-line wrapper around `cfg.get`. - **Primitive Obsession (mild)**: `basis, now, before = "TOTAL", ...` infers `basis` as `str`, not `ComparisonBasis`. ### UI - **P3 `index.html:799-803`**: new copy ("STATISTICS COMPARISONS", "MIN COVERAGE…") has no `data-i18n` (159 uses elsewhere in `index.html`); neighbouring admin config labels aren't translated either, so consistent with the form — but hard-coded English with bounds in the label. ### Spec note `docs/api/README.md` says the rule covers "a week or month"; the code applies it to days too. Equivalent outcome, but the docs should say days are never partial. ## Spec **Verdict:** no P1/P2. Every decision in #104 is in the code; nothing goes against the spec. ### (a) Missing or partial None. - *"New config key … default 0.80, validated to 0.5–1.0 … editable"*: column migration `app/db/database.py:247`, named constant and Field in `app/schemas/occupancy_models.py`, admin input `index.html:796`. - *"Coverage = covered business days / business days (Closed Days excluded)"*: `PeriodQuality.coverage` (`app/schemas/statistics.py:36`); the denominator already excludes Closed Days. - *"Both complete → totals / ≥ threshold → daily averages … basis / below → PARTIAL_PERIOD"*: `compare_visitors` (`analytics_service.py:108-143`), using `>=`, so a period exactly at the threshold is compared. - *"Applies to the summary comparisons and the entrance comparisons"*: covers `previous`, `last_year`, `same_weekday_last_week`, and entrance `previous` / `last_year`. ### (b) Scope creep - **P3**: `reference_daily_average` (`statistics.py:129`) is a contract addition; harmless. For months it repeats `previous_month_daily_average` (`analytics_service.py:~418`), which still divides by covered days whatever the coverage — two sources for the same number. Note it in the docs or retire one later. ### (c) Implemented but wrong Nothing confirmed. - **P3, `analytics_service.py:258`**: the `granularity != "day"` exemption is gone. A day is covered (1/1) or not (0/1), and a reference day with no data exits earlier as `NO_DATA_*`, so nothing changes — unless counted-flow `has_data` and cycle-quality `has_data` can disagree for a day. No test pins the day path under the new rule. - Checked OK: a partial side with `covered_days` 0 has coverage 0 → `PARTIAL_PERIOD`, no division; a fully closed current period returns early; `last_year` omission unchanged (`PARTIAL_PERIOD` still carries `reference_visitors`); `usual_weekday` correctly out of scope. ### Tests The 5 new tests fail without the change (`basis` didn't exist; the old rule gave `PARTIAL_PERIOD` for 6/7). 33 statistics tests pass in a throwaway worktree. An ad-hoc probe with the **current** week partial (6/7) vs a complete reference gave `DAILY_AVERAGE`, −16.7% on summary and entrances — correct, but not in the suite. Gaps (**P3**, `tests/test_statistics_comparison_coverage.py`): 1. No test with the current side partial (only the reference side). 2. No month test, and no `last_year` with `DAILY_AVERAGE`. 3. No test combining Closed Days with coverage (e.g. 1 Closed Day + 1 missing day → 5/6 → `DAILY_AVERAGE`). 4. No test pinning the exact threshold (e.g. 4/5 at 0.80) to lock in `>=`. 5. The stricter-threshold case asserts only on the summary, not the entrance comparison. ### Minor Spec: *"the partial badge shows"*. `quality.partial` stays true; no statistics UI renders it yet, so only the consumer can check it. --- **Summary:** Standards 11 (2×P2, 9×P3), worst: `compare_visitors` can compare a total against a daily average when `current_quality` is None (latent today), plus bare 0.5/1.0 bounds copied in four places. Spec 8 (all P3), worst: no test with the current side partial or with Closed Days + coverage. 🤖 Generated with [Claude Code](https://claude.com/claude-code) 
- `compare_visitors` takes `PeriodVisitors(visitors, quality)` pairs and the
  current period's quality is always passed, so a total can no longer be
  compared with a daily average; `basis` is typed `ComparisonBasis`.
- The threshold's bounds are named (`MIN_COMPARISON_COVERAGE_FLOOR/CEILING`)
  and the stored value is clamped to them, so a bad row cannot divide by
  zero.
- The admin label is translated (es/en) and no longer repeats the bounds.
- Docs: days are never partial; `reference_daily_average` vs
  `previous_month_daily_average`.
- Tests: current side partial, month and last-year daily averages, Closed
  Days in coverage with the inclusive threshold (mutation-checked), the
  stricter threshold on entrances too, and config saves that send only the
  threshold (calibration-owned fields are rejected since #115).

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
gabogg commented 2026-09-25 18:58:27 +00:00
Author
Owner
Copy link

Pass-1 findings addressed in 0b6df9b (merge of the updated #115) and the following fix commit. Full suite 349 passed; frontend 80/80; ruff and docs check clean.

Standards

Finding Fix
P2 bare 0.5/1.0 bounds MIN_COMPARISON_COVERAGE_FLOOR / _CEILING in occupancy_models.py, used by the schema Field, the service clamp and the tests. The HTML input keeps min/max attributes (browser validation, same as the trust_* inputs) but the bounds are gone from the label text.
P3 value="0.80" / DEFAULT 0.80 copies Kept, consistent with the existing trust_* rows (SQL DDL and HTML can't import the constant); noted here.
P2 total vs daily average when current_quality is None current_quality is now required on statistics_visitor_comparison_async; the day comparison passes the day's quality.
P3 division by zero on a stored 0 _min_comparison_coverage clamps the stored value to the named bounds, so both sides cover ≥ 1 day before dividing.
Data Clumps (count, quality) PeriodVisitors(visitors, quality) NamedTuple with per_day(); compare_visitors(reference_period, current, reference, min_coverage).
Repeated None checks Gone with the required quality.
Middle Man _min_comparison_coverage Now does the clamping, so it earns its place.
basis inferred as str Annotated ComparisonBasis.
P3 untranslated UI copy occupancy.statisticsComparisonsTitle / minComparisonCoverageLabel in es and en.
Docs: "week or month" README states a day is never partial.

Spec

Finding Fix
P3 reference_daily_average vs previous_month_daily_average Documented as equal for monthly daily-average comparisons; the month field is kept for the month view.
P3 day path under the new rule untested Covered implicitly: day comparisons now always pass quality; a day is 1/1 or 0/1 (documented).
Test gaps 1–5 Current side partial (parametrized with reference side); month last_year by daily average; Closed Days + coverage at exactly 0.80 (fails if >= becomes >, mutation-checked); stricter threshold asserted on summary and entrances.

🤖 Generated with Claude Code

Pass-1 findings addressed in `0b6df9b` (merge of the updated #115) and the following fix commit. Full suite 349 passed; frontend 80/80; ruff and docs check clean. ### Standards | Finding | Fix | |---|---| | P2 bare `0.5/1.0` bounds | `MIN_COMPARISON_COVERAGE_FLOOR` / `_CEILING` in `occupancy_models.py`, used by the schema Field, the service clamp and the tests. The HTML input keeps `min`/`max` attributes (browser validation, same as the `trust_*` inputs) but the bounds are gone from the label text. | | P3 `value="0.80"` / `DEFAULT 0.80` copies | Kept, consistent with the existing `trust_*` rows (SQL DDL and HTML can't import the constant); noted here. | | P2 total vs daily average when `current_quality is None` | `current_quality` is now required on `statistics_visitor_comparison_async`; the day comparison passes the day's quality. | | P3 division by zero on a stored 0 | `_min_comparison_coverage` clamps the stored value to the named bounds, so both sides cover ≥ 1 day before dividing. | | Data Clumps (count, quality) | `PeriodVisitors(visitors, quality)` NamedTuple with `per_day()`; `compare_visitors(reference_period, current, reference, min_coverage)`. | | Repeated `None` checks | Gone with the required quality. | | Middle Man `_min_comparison_coverage` | Now does the clamping, so it earns its place. | | `basis` inferred as `str` | Annotated `ComparisonBasis`. | | P3 untranslated UI copy | `occupancy.statisticsComparisonsTitle` / `minComparisonCoverageLabel` in es and en. | | Docs: "week or month" | README states a day is never partial. | ### Spec | Finding | Fix | |---|---| | P3 `reference_daily_average` vs `previous_month_daily_average` | Documented as equal for monthly daily-average comparisons; the month field is kept for the month view. | | P3 day path under the new rule untested | Covered implicitly: day comparisons now always pass quality; a day is 1/1 or 0/1 (documented). | | Test gaps 1–5 | Current side partial (parametrized with reference side); month `last_year` by daily average; Closed Days + coverage at exactly 0.80 (fails if `>=` becomes `>`, mutation-checked); stricter threshold asserted on summary **and** entrances. | 🤖 Generated with [Claude Code](https://claude.com/claude-code)
gabogg commented 2026-09-25 19:14:02 +00:00
Author
Owner
Copy link

Code review — pass 2 (two-axis)

Re-reviewed at ace941e. Each axis verified every pass-1 finding against the code, then reviewed the fix commit(s) 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 head 7cf4a0b; fix commit ace941e. Ruff check/format clean; tests/test_statistics_comparison_coverage.py 8/8 and the statistics/occupancy/analytics subset 98/98 (throwaway worktree).

Pass-1 findings

Finding Verdict Evidence
P2 bare ge=0.5, le=1.0 bounds ✅ fixed MIN_COMPARISON_COVERAGE_FLOOR/CEILING (occupancy_models.py:193-194), used at :335-336, in the service clamp (analytics_service.py:232) and tests; bounds removed from the label. HTML min/max and README 0.5–1.0 stay literals, like trust_*.
P3 value="0.80" / DEFAULT 0.80 copies ⊘ declined-with-reason SQL DDL and HTML can't import the constant; same pattern as trust_*. Accepted.
P2 total vs daily average when current_quality is None ✅ fixed current_quality: PeriodQuality required (:252); the day path passes quality (:391); no None branch left.
P3 division by zero on a stored 0 ✅ fixed Stored value clamped to FLOOR/CEILING (:227-232). See N1.
Data Clumps (count, quality) ✅ fixed PeriodVisitors(visitors, quality) with per_day() (:111-119), used at all 3 sites.
Repeated Switches on None ✅ fixed Removed with the optional quality.
Middle Man _min_comparison_coverage ✅ fixed Now does the clamping.
Primitive Obsession basis: str ✅ fixed basis: ComparisonBasis (:134).
P3 untranslated UI copy ✅ fixed data-i18n at index.html:800,802 (label text in a <span>, so the input survives); keys under occupancy in es and en.
Docs: "week or month" ✅ fixed README: "A day is never partial…".

New findings

  • P3 (N1) analytics_service.py:137: comment overstates its guarantee. It says min_coverage clamped above zero means both sides cover ≥ 1 day, but PeriodQuality.coverage returns 1.0 when business_days == 0, so a fully closed side with covered_days=0 passes when the other side is partial, and per_day() would divide by zero. Unreachable today (the summary returns early on CLOSED_PERIOD; entrances skip closed cycles so event_count is 0). Guard in per_day()/compare_visitors, or reword to name the callers' guard.
  • P3 the clamp has no test: only the Pydantic rejection is tested, not an out-of-range stored row (the pass-1 scenario).
  • P3 tests/test_statistics_comparison_coverage.py:210: upper out-of-range case 1.1 is a bare literal; use MIN_COMPARISON_COVERAGE_CEILING + 0.1.
  • P3 (smell) weak test :94-96: test_threshold_and_its_bounds_are_named only checks the constants are ordered.

Merge readiness (this axis): Ready. All pass-1 P2s fixed; no P1/P2. P3s → follow-up issue.

Spec

Own diff only, fix commit ace941e. 20 statistics tests pass (_comparison_coverage, _summary, _closed_days) in a throwaway worktree; three mutation checks run there.

Pass-1 findings

Finding Verdict Evidence
P3 reference_daily_average duplicates previous_month_daily_average ⊘ declined with reason Documented as equal (docs/api/README.md:134); both compute round(reference / covered_days, 1).
P3 day path under the new rule untested ✅ The claim holds: the current day is 404 without data or closed and returns early; a closed or empty reference day returns CLOSED_PERIOD / NO_DATA_* before compare_visitors; both has_data sources use the same EXISTS over the same camera filter and bounds (occupancy_repository.py:1260, :1317). And it is pinned: forcing every day comparison to PARTIAL_PERIOD fails tests/test_statistics_summary.py:225.
Gap 1: current side partial ✅ test_a_partial_side_above_threshold_compares_daily_averages[current]
Gap 2: month / last_year with DAILY_AVERAGE ✅ test_last_year_and_months_use_the_same_rule
Gap 3: Closed Days combined with coverage ✅ test_closed_days_count_towards_coverage_and_the_threshold_is_inclusive (2 Closed Days, 4/5)
Gap 4: exact threshold pinned ✅ Same test fails when >= becomes > (mutation).
Gap 5: stricter threshold on entrances ✅ Asserted on summary and entrance comparison, both sides.
Minor: "the partial badge shows" ⊘ quality.partial stays true; no statistics UI renders it yet.

Other checks: forcing the daily-average branch always fails test_complete_periods_compare_totals (TOTAL pinned). All five call sites route through compare_visitors. Spec decisions re-checked against the fix commit: "default 0.80, validated to 0.5–1.0" (named bounds + clamp), "below the threshold: change_percent: null, reason PARTIAL_PERIOD", "Applies to the summary comparisons and the entrance comparisons" — all met.

New findings

  • P3 docs/api/README.md:134: "Each comparison states its basis" — basis is null on withheld comparisons (PARTIAL_PERIOD, NO_DATA_*, CLOSED_PERIOD) and on usual_weekday, which does compute a change_percent. Docs precision only; suggest "each calculated comparison except usual_weekday".
  • P3 the Closed Days / exact-threshold test asserts only on the summary, not the entrance comparison. Low risk (same function).

Merge readiness (this axis): Ready. No P1/P2; P3s → follow-up issue.

Summary: Standards: no P1/P2; 4 P3 (worst: per_day() comment overstates its zero-division guarantee). Spec: no P1/P2; 2 P3 (worst: docs say every comparison states its basis).

🤖 Generated with Claude Code

# Code review — pass 2 (two-axis) Re-reviewed at `ace941e`. Each axis verified every pass-1 finding against the code, then reviewed the fix commit(s) 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 head 7cf4a0b; fix commit ace941e. Ruff check/format clean; `tests/test_statistics_comparison_coverage.py` 8/8 and the statistics/occupancy/analytics subset 98/98 (throwaway worktree). ### Pass-1 findings | Finding | Verdict | Evidence | |---|---|---| | P2 bare `ge=0.5, le=1.0` bounds | ✅ fixed | `MIN_COMPARISON_COVERAGE_FLOOR/CEILING` (`occupancy_models.py:193-194`), used at `:335-336`, in the service clamp (`analytics_service.py:232`) and tests; bounds removed from the label. HTML `min`/`max` and README `0.5–1.0` stay literals, like `trust_*`. | | P3 `value="0.80"` / `DEFAULT 0.80` copies | ⊘ declined-with-reason | SQL DDL and HTML can't import the constant; same pattern as `trust_*`. Accepted. | | P2 total vs daily average when `current_quality is None` | ✅ fixed | `current_quality: PeriodQuality` required (`:252`); the day path passes `quality` (`:391`); no `None` branch left. | | P3 division by zero on a stored 0 | ✅ fixed | Stored value clamped to FLOOR/CEILING (`:227-232`). See N1. | | Data Clumps (count, quality) | ✅ fixed | `PeriodVisitors(visitors, quality)` with `per_day()` (`:111-119`), used at all 3 sites. | | Repeated Switches on `None` | ✅ fixed | Removed with the optional quality. | | Middle Man `_min_comparison_coverage` | ✅ fixed | Now does the clamping. | | Primitive Obsession `basis: str` | ✅ fixed | `basis: ComparisonBasis` (`:134`). | | P3 untranslated UI copy | ✅ fixed | `data-i18n` at `index.html:800,802` (label text in a `<span>`, so the input survives); keys under `occupancy` in es and en. | | Docs: "week or month" | ✅ fixed | README: "A day is never partial…". | ### New findings - **P3 (N1) `analytics_service.py:137`: comment overstates its guarantee.** It says min_coverage clamped above zero means both sides cover ≥ 1 day, but `PeriodQuality.coverage` returns `1.0` when `business_days == 0`, so a fully closed side with `covered_days=0` passes when the other side is partial, and `per_day()` would divide by zero. Unreachable today (the summary returns early on `CLOSED_PERIOD`; entrances skip closed cycles so `event_count` is 0). Guard in `per_day()`/`compare_visitors`, or reword to name the callers' guard. - **P3 the clamp has no test**: only the Pydantic rejection is tested, not an out-of-range stored row (the pass-1 scenario). - **P3 `tests/test_statistics_comparison_coverage.py:210`**: upper out-of-range case `1.1` is a bare literal; use `MIN_COMPARISON_COVERAGE_CEILING + 0.1`. - **P3 (smell) weak test `:94-96`**: `test_threshold_and_its_bounds_are_named` only checks the constants are ordered. **Merge readiness (this axis):** Ready. All pass-1 P2s fixed; no P1/P2. P3s → follow-up issue. ## Spec Own diff only, fix commit `ace941e`. 20 statistics tests pass (`_comparison_coverage`, `_summary`, `_closed_days`) in a throwaway worktree; three mutation checks run there. ### Pass-1 findings | Finding | Verdict | Evidence | |---|---|---| | P3 `reference_daily_average` duplicates `previous_month_daily_average` | ⊘ declined with reason | Documented as equal (`docs/api/README.md:134`); both compute `round(reference / covered_days, 1)`. | | P3 day path under the new rule untested | ✅ | The claim holds: the current day is 404 without data or closed and returns early; a closed or empty reference day returns `CLOSED_PERIOD` / `NO_DATA_*` before `compare_visitors`; both `has_data` sources use the same `EXISTS` over the same camera filter and bounds (`occupancy_repository.py:1260`, `:1317`). And it is pinned: forcing every day comparison to `PARTIAL_PERIOD` fails `tests/test_statistics_summary.py:225`. | | Gap 1: current side partial | ✅ | `test_a_partial_side_above_threshold_compares_daily_averages[current]` | | Gap 2: month / `last_year` with `DAILY_AVERAGE` | ✅ | `test_last_year_and_months_use_the_same_rule` | | Gap 3: Closed Days combined with coverage | ✅ | `test_closed_days_count_towards_coverage_and_the_threshold_is_inclusive` (2 Closed Days, 4/5) | | Gap 4: exact threshold pinned | ✅ | Same test fails when `>=` becomes `>` (mutation). | | Gap 5: stricter threshold on entrances | ✅ | Asserted on summary **and** entrance comparison, both sides. | | Minor: "the partial badge shows" | ⊘ | `quality.partial` stays true; no statistics UI renders it yet. | Other checks: forcing the daily-average branch always fails `test_complete_periods_compare_totals` (`TOTAL` pinned). All five call sites route through `compare_visitors`. Spec decisions re-checked against the fix commit: *"default 0.80, validated to 0.5–1.0"* (named bounds + clamp), *"below the threshold: `change_percent: null`, reason `PARTIAL_PERIOD`"*, *"Applies to the summary comparisons and the entrance comparisons"* — all met. ### New findings - **P3 `docs/api/README.md:134`**: *"Each comparison states its `basis`"* — `basis` is null on withheld comparisons (`PARTIAL_PERIOD`, `NO_DATA_*`, `CLOSED_PERIOD`) and on `usual_weekday`, which does compute a `change_percent`. Docs precision only; suggest "each calculated comparison except `usual_weekday`". - **P3** the Closed Days / exact-threshold test asserts only on the summary, not the entrance comparison. Low risk (same function). **Merge readiness (this axis):** Ready. No P1/P2; P3s → follow-up issue. --- **Summary:** Standards: no P1/P2; 4 P3 (worst: `per_day()` comment overstates its zero-division guarantee). Spec: no P1/P2; 2 P3 (worst: docs say every comparison states its `basis`). 🤖 Generated with [Claude Code](https://claude.com/claude-code)
gabogg changed target branch from fix/occupancy-config-save-preserves-calibration to master 2026-09-25 19:17:06 +00:00
gabogg commented 2026-09-25 19:17:12 +00:00
Author
Owner
Copy link

Pass-2 blockers fixed; remaining P3s filed as #120. Combined master + #115 + #116: pytest 351 passed, 1 skipped; frontend 80/80. Merging.

🤖 Generated with Claude Code

Pass-2 blockers fixed; remaining P3s filed as #120. Combined master + #115 + #116: pytest 351 passed, 1 skipped; frontend 80/80. Merging. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
gabogg merged commit c34e7620c0 into master 2026-09-25 19:17:14 +00:00
gabogg deleted branch feat/statistics-comparison-coverage 2026-09-25 19:17:14 +00:00
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
blocked
Waits on the issues listed in its "Blocked by:" line before implementation

  
bug
Category: something is broken; behaviour differs from the spec or docs

  
enhancement
Category: new feature or improvement, including refactors, chores and docs

  
high-priority
Address before ordinary-priority work; independent of triage readiness and blockers

  
low-priority
Address after ordinary-priority work; independent of triage readiness and blockers

  
needs-info
Waiting on reporter for more information

  
needs-triage
Maintainer needs to evaluate this issue

  
ready-for-agent
Fully specified, ready for an AFK agent

  
ready-for-human
Requires human implementation

  
referenced
Previously closed issue reopened or referenced by an open issue; visible history marker

  
research
Needs investigation (internet or local resources) before it can be triaged down; used alongside a category and a triage role

  
wontfix
Will not be actioned

No labels
blocked
bug
enhancement
high-priority
low-priority
needs-info
needs-triage
ready-for-agent
ready-for-human
referenced
research
wontfix
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
gabogg/hikcentral!116
No description provided.