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

gabogg commented

2026-09-25 12:14:44 +00:00

Owner

Follow-ups from the pass-2 review of #90 (comment on #90). All P3; the P2s were fixed in d710ba8 before merge.

Standards

Repeated active-business-day expression. datetime.date.fromisoformat(facility_cycle_bounds(None, reset).label) recurs in analytics_service.py; extract active_business_day(reset). validate_closed_statistics_day_async re-reads config the caller already read.
Repeated ValueError → 422 blocks in statistics_controller.py and analytics_controller.py. One helper or exception handler.
Primitive Obsession. The service takes baseline: str | None while the controllers use Literal["same_weekday_4w"]; UsualHourlyBaseline.reason / UsualDaypartsBaseline.reason are bare str. Share one Literal alias in app/schemas/.
N+1 on baseline samples. Each sample re-runs the full get_hourly_timeseries_async / get_dwell_dayparts_async. Fine for 4 samples; a narrower per-day aggregate would be clearer.
Verdict-priority CASE duplicated. is_cycle_data_trusted_async and get_trusted_calibration_history_async both spell out RETROACTIVE > MANUAL > AUTOMATIC. Extract one SQL fragment.
Dead branch. if shares else 0.0 in the daypart baseline can't be reached now that usable requires entries.
Fragile float assertion. tests/test_statistics_baselines.py compares a sum of rounded shares with == 100.0; use pytest.approx.
Validate bucket size before computing. The baseline bucket-size check runs after the target day's full bucket computation.

Spec

Second INSUFFICIENT_USUAL_WEEKDAYS branch untested (candidates exist but fewer than 2 are usable), and mean_share_percent is only checked in a case where every share is 100%.
Equal dayparts across changed opening hours. Equal edges come from each source day's own open window. If hours changed within the 8-week lookback, parts are averaged by position with different cut times. Consider dropping source days whose edges differ from the target day's, and/or adding a label to UsualDaypart.
Wrong parameter name in a test. Two assertions call /api/analytics/dwell/dayparts with date=; the route parameter is day.
Hourly baseline has no guard for differing bucket labels across source days (fine for fixed clock buckets).

🤖 Generated with Claude Code

Follow-ups from the pass-2 review of #90 (comment on #90). All P3; the P2s were fixed in d710ba8 before merge. ## Standards - [ ] **Repeated active-business-day expression.** `datetime.date.fromisoformat(facility_cycle_bounds(None, reset).label)` recurs in `analytics_service.py`; extract `active_business_day(reset)`. `validate_closed_statistics_day_async` re-reads config the caller already read. - [ ] **Repeated `ValueError → 422` blocks** in `statistics_controller.py` and `analytics_controller.py`. One helper or exception handler. - [ ] **Primitive Obsession.** The service takes `baseline: str | None` while the controllers use `Literal["same_weekday_4w"]`; `UsualHourlyBaseline.reason` / `UsualDaypartsBaseline.reason` are bare `str`. Share one `Literal` alias in `app/schemas/`. - [ ] **N+1 on baseline samples.** Each sample re-runs the full `get_hourly_timeseries_async` / `get_dwell_dayparts_async`. Fine for 4 samples; a narrower per-day aggregate would be clearer. - [ ] **Verdict-priority `CASE` duplicated.** `is_cycle_data_trusted_async` and `get_trusted_calibration_history_async` both spell out RETROACTIVE > MANUAL > AUTOMATIC. Extract one SQL fragment. - [ ] **Dead branch.** `if shares else 0.0` in the daypart baseline can't be reached now that `usable` requires entries. - [ ] **Fragile float assertion.** `tests/test_statistics_baselines.py` compares a sum of rounded shares with `== 100.0`; use `pytest.approx`. - [ ] **Validate bucket size before computing.** The baseline bucket-size check runs after the target day's full bucket computation. ## Spec - [ ] **Second `INSUFFICIENT_USUAL_WEEKDAYS` branch untested** (candidates exist but fewer than 2 are usable), and `mean_share_percent` is only checked in a case where every share is 100%. - [ ] **Equal dayparts across changed opening hours.** Equal edges come from each source day's own open window. If hours changed within the 8-week lookback, parts are averaged by position with different cut times. Consider dropping source days whose edges differ from the target day's, and/or adding a label to `UsualDaypart`. - [ ] **Wrong parameter name in a test.** Two assertions call `/api/analytics/dwell/dayparts` with `date=`; the route parameter is `day`. - [ ] **Hourly baseline has no guard for differing bucket labels** across source days (fine for fixed clock buckets). 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg added the

needs-triage

label

2026-09-25 12:14:44 +00:00

gabogg referenced this issue from a commit

2026-09-25 12:17:25 +00:00

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

gabogg referenced this issue

2026-09-25 17:29:06 +00:00

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

gabogg commented

2026-09-25 17:29:11 +00:00

Author

Owner

The opening-hours question is settled in #105 (only compare days with matching opening hours); daypart clock labels moved to #106. The rest stays here.

🤖 Generated with Claude Code

The opening-hours question is settled in #105 (only compare days with matching opening hours); daypart clock labels moved to #106. The rest stays here. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg added

ready-for-agent

and removed

needs-triage

labels

2026-09-25 17:29:24 +00:00

gabogg referenced this issue

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

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

gabogg referenced this issue

2026-09-26 01:41:03 +00:00

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