refactor(occupancy): PR #54 review follow-ups — third copy of the counted-camera rule, window-end convention, test edges #66

New issue

Closed

opened 2026-09-23 22:15:43 +00:00 by gabogg · 0 comments

gabogg commented

2026-09-23 22:15:43 +00:00

Owner

Non-blocking follow-ups from the round-2 review of PR #54 (merged). None blocks correctness today. Line numbers refer to PR #54's head 990d495; re-check them after merge.

Code quality (Standards)

Third copy of the counted-camera rule. _COUNTED_CAMERA_JOIN / _COUNTED_CAMERA_FILTER (app/db/occupancy_repository.py:24-26) carry a comment saying every published figure goes through the one rule. But the top-entrances and top-exits lists re-apply it in Python (not c["is_excluded"] and c["is_active"], around :1061 and :1084), so that copy can drift. Pre-existing. Either derive those lists from rows the SQL already filtered, or narrow the comment.
Window-end mismatch. get_hourly_flow_distribution_async now ends at <= cycle_end, while get_bucketed_cycle_flow_async still uses < ? (:1608). Decide one convention for cycle windows. #58 moves trust onto the bucketed query, so it must keep parity (noted on #58).
Test helper takes the timestamp two ways. tests/test_trust_dataset_parity.py:27-30 _insert_event takes both hour and at, and at silently overrides hour, so callers pass a dummy hour=0. Use one epoch argument, plus a helper that computes it from an hour.
Glossary wording. CONTEXT.md Camera Group States → Multi-camera group: "…which ADR 0005's one-camera-per-group rule does not allow for. It is still counted…" reads as a contradiction. Use "violates" or "does not account for".

Tests (Spec)

The boundary test uses an edge no caller passes. tests/test_trust_dataset_parity.py:15, :108-116 sets CYCLE_END = CYCLE_START + 24*3600. Real callers pass start + 86400 - 0.001 or reset - 0.001. With the test's value, the end-edge event lands in a 25th bucket (hour_idx = 24) that the volume-sum assertion can't see, and it pins a convention where a boundary event counts in two adjacent cycles. Test with the production edge (- 0.001), and assert the bucket indices as well as the volumes.

Acceptance criteria

Items 1–5 fixed, or explicitly declined with a reason.
Full suite green.

Related: #35, #58, PR #54.

> Non-blocking follow-ups from the round-2 review of PR #54 (merged). None blocks correctness today. Line numbers refer to PR #54's head `990d495`; re-check them after merge. ## Code quality (Standards) 1. **Third copy of the counted-camera rule.** `_COUNTED_CAMERA_JOIN` / `_COUNTED_CAMERA_FILTER` (`app/db/occupancy_repository.py:24-26`) carry a comment saying every published figure goes through the one rule. But the top-entrances and top-exits lists re-apply it in Python (`not c["is_excluded"] and c["is_active"]`, around `:1061` and `:1084`), so that copy can drift. Pre-existing. Either derive those lists from rows the SQL already filtered, or narrow the comment. 2. **Window-end mismatch.** `get_hourly_flow_distribution_async` now ends at `<= cycle_end`, while `get_bucketed_cycle_flow_async` still uses `< ?` (`:1608`). Decide one convention for cycle windows. #58 moves trust onto the bucketed query, so it must keep parity (noted on #58). 3. **Test helper takes the timestamp two ways.** `tests/test_trust_dataset_parity.py:27-30` `_insert_event` takes both `hour` and `at`, and `at` silently overrides `hour`, so callers pass a dummy `hour=0`. Use one `epoch` argument, plus a helper that computes it from an hour. 4. **Glossary wording.** `CONTEXT.md` *Camera Group States* → *Multi-camera group*: "…which ADR 0005's one-camera-per-group rule does not allow for. It is still counted…" reads as a contradiction. Use "violates" or "does not account for". ## Tests (Spec) 5. **The boundary test uses an edge no caller passes.** `tests/test_trust_dataset_parity.py:15, :108-116` sets `CYCLE_END = CYCLE_START + 24*3600`. Real callers pass `start + 86400 - 0.001` or `reset - 0.001`. With the test's value, the end-edge event lands in a 25th bucket (`hour_idx = 24`) that the volume-sum assertion can't see, and it pins a convention where a boundary event counts in two adjacent cycles. Test with the production edge (`- 0.001`), and assert the bucket indices as well as the volumes. ## Acceptance criteria - [ ] Items 1–5 fixed, or explicitly declined with a reason. - [ ] Full suite green. Related: #35, #58, PR #54.

gabogg referenced this issue

2026-09-23 22:16:09 +00:00

fix(occupancy): trust/KPI dataset parity and 1:1 camera-group invariant — phase 2 (#28, #35) #54

gabogg referenced this issue

2026-09-23 22:16:12 +00:00

refactor(occupancy): one query shape for counts-over-time; delete get_hourly_flow_distribution_async #58

gabogg referenced this issue from a commit

2026-09-23 22:16:33 +00:00

fix(occupancy): trust/KPI dataset parity and 1:1 camera-group invariant — phase 2 (#54)

gabogg changed title from ~~Follow-ups from PR #54 review: third copy of the counted-camera rule, window-end convention, test edges~~ to refactor(occupancy): PR #54 review follow-ups — third copy of the counted-camera rule, window-end convention, test edges

2026-09-24 10:15:37 +00:00

gabogg added the

ready-for-agent

label

2026-09-24 10:15:38 +00:00

gabogg referenced this issue

2026-09-24 23:05:38 +00:00

docs: audit accuracy, measure coverage, and reorganize #79

gabogg referenced this issue from a commit

2026-09-25 18:48:44 +00:00

fix(occupancy): address review follow-ups from PR #53 and #54 (#65, #66)

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