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

New issue

Closed

opened 2026-10-02 23:51:06 +00:00 by gabogg · 0 comments

gabogg commented

2026-10-02 23:51:06 +00:00

Owner

Standards

A helper and a method have nearly the same name (analytics_service.py). The module helper _trusted_cycle_multiplier(log) returns float | None. The method _trusted_cycle_multiplier_async(day, bounds, default) returns the default instead of None.
- Acceptance: rename the helper so the two can't be confused, e.g. _plausible_trusted_multiplier.
The typed record isn't used where it's read. The trusted-multiplier helpers still take Mapping[str, Any] rather than CalibrationLogRecord.
- Acceptance: the helpers take CalibrationLogRecord, so the type is checked where it's consumed.
CalibrationLogRecord is incomplete (occupancy_models.py).
- The batched query's SELECT * also returns rank_in_cycle, which the TypedDict omits.
- is_trusted is typed int, but the column has no NOT NULL constraint.
- Acceptance: the record matches every column the query returns, with nullability matching the schema.
Duplicated code in the repository. Four copies of the same comprehension that flattens cycle parameters, in the four batch methods.
- Acceptance: one helper, e.g. _cycle_values_params(cycles), used by all four.
Duplicated code in the tests (tests/test_statistics_periods.py). The two EXPLAIN-plan assertion blocks are copies of each other.
- Acceptance: one _assert_indexed_plan(sql, params) helper, used for both queries.

Spec

A test name describes old behaviour. test_hourly_baseline_appends_duplicate_clock_labels_on_dst_fallback now checks positional averaging, and the plan doc cites the old name.
- Acceptance: the name says what it checks (positional averaging for duplicated labels), and the plan doc is updated to match.
#97 left open. The PR said it "addresses #96 and #97" but closed only #124. Every #97 item is done, and its daypart item moved to #105/#106. #97 was closed by hand after the merge.
- Acceptance: nothing further in code. Kept here only as a record.

Context: measured performance after the fix

The batched queries plan as SEARCH e USING INDEX idx_counting_events_cam_range, with no MATERIALIZE. On the production aiosqlite path:

they beat per-day queries on sparse 365-day data (59 ms vs 68 ms for flow);
they are within 3–13% of per-day on dense 62-day data;
pass 2's regression was 4–20×.

The remaining per-day peak and average Riemann work is tracked in #203.

Refs #167, #96, #97, #124, #203.

🤖 Generated with Claude Code

Deferred P3 findings from the third review pass on #167 (baseline and query follow-ups for #96, #97 and #124). None of them blocked the merge. Under the repo rule, a follow-up PR's third-pass P3s become one linked issue. ## Standards 1. **A helper and a method have nearly the same name** (`analytics_service.py`). The module helper `_trusted_cycle_multiplier(log)` returns `float | None`. The method `_trusted_cycle_multiplier_async(day, bounds, default)` returns the default instead of None. - *Acceptance:* rename the helper so the two can't be confused, e.g. `_plausible_trusted_multiplier`. 2. **The typed record isn't used where it's read.** The trusted-multiplier helpers still take `Mapping[str, Any]` rather than `CalibrationLogRecord`. - *Acceptance:* the helpers take `CalibrationLogRecord`, so the type is checked where it's consumed. 3. **`CalibrationLogRecord` is incomplete** (`occupancy_models.py`). - The batched query's `SELECT *` also returns `rank_in_cycle`, which the TypedDict omits. - `is_trusted` is typed `int`, but the column has no NOT NULL constraint. - *Acceptance:* the record matches every column the query returns, with nullability matching the schema. 4. **Duplicated code in the repository.** Four copies of the same comprehension that flattens cycle parameters, in the four batch methods. - *Acceptance:* one helper, e.g. `_cycle_values_params(cycles)`, used by all four. 5. **Duplicated code in the tests** (`tests/test_statistics_periods.py`). The two EXPLAIN-plan assertion blocks are copies of each other. - *Acceptance:* one `_assert_indexed_plan(sql, params)` helper, used for both queries. ## Spec 6. **A test name describes old behaviour.** `test_hourly_baseline_appends_duplicate_clock_labels_on_dst_fallback` now checks positional averaging, and the plan doc cites the old name. - *Acceptance:* the name says what it checks (positional averaging for duplicated labels), and the plan doc is updated to match. 7. **#97 left open.** The PR said it "addresses #96 and #97" but closed only #124. Every #97 item is done, and its daypart item moved to #105/#106. #97 was closed by hand after the merge. - *Acceptance:* nothing further in code. Kept here only as a record. ## Context: measured performance after the fix The batched queries plan as `SEARCH e USING INDEX idx_counting_events_cam_range`, with no `MATERIALIZE`. On the production `aiosqlite` path: - they beat per-day queries on sparse 365-day data (59 ms vs 68 ms for flow); - they are within 3–13% of per-day on dense 62-day data; - pass 2's regression was 4–20×. The remaining per-day peak and average Riemann work is tracked in #203. Refs #167, #96, #97, #124, #203. 🤖 Generated with [Claude Code](https://claude.com/claude-code)

gabogg added the

ready-for-agent

low-priority

labels

2026-10-02 23:51:06 +00:00

gabogg referenced this issue

2026-10-02 23:52:23 +00:00

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

gabogg referenced this issue

2026-10-02 23:52:26 +00:00

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

gabogg referenced this issue from a pull request that will close it,