fix(occupancy): PR #69 review follow-ups — quiet windows away from the reset, cycle-bounds API, CI timezone #76

New issue

Open

opened 2026-09-24 22:31:47 +00:00 by gabogg · 0 comments

gabogg commented

2026-09-24 22:31:47 +00:00

Owner

Findings from the round-3 review of PR #69 (comment 1721). Includes one accepted P2 that needs a design decision, hence needs-triage.

Accepted P2 (needs a decision)

calibration_cycle_bounds picks the cycle by calendar day. A 21:00–22:00 window calibrates the cycle that ended 17 h earlier. A 23:30–00:30 window waits until midnight and then reads the cycle still in progress. Production (03:30–04:30 around 04:00) is unaffected, and a 3-day minute-by-minute simulation found no skipped or doubled calibration there. The constraint is documented in facility_time.calibration_cycle_bounds, the config field description and docs/architecture/admin-data-visualization.md. Decide: support arbitrary windows (choose the most recently completed cycle whose end precedes the window), or validate the config so the window must straddle or follow the reset.

Behaviour (P3)

With the fresh default daily_reset_time = "00:00", a venue open past midnight (10:00–02:00) is still reported closed after 00:00, and events are stamped is_working_hours = 0. The business-day fix only helps when the reset is at or after closing.
A holiday on the calendar day doesn't apply between 00:00 and the reset. Consistent with the business-day model, but it should be documented.
A pre-reset calibration logs a future last_calibrated_at (cycle_end − 900); e.g. a run at 02:00 is logged as 03:45.
Display strings (timestamp_formatted, peak_time_formatted) still use server time (time.localtime), not facility time.

Code quality (P3)

The thin wrappers get_business_day_epoch_bounds / get_completed_business_cycle_bounds / the new get_calibration_cycle_bounds: every caller is a service or a test (the round-2 justification citing AGENTS.md doesn't hold). Their only real use is an argument-order shim for old tests. Inline them or return CycleBounds; they currently declare tuple returns.
facility_window_containing returns a raw pair read as window[1]. Add a small Window(start, end).
date.fromisoformat(label) is repeated. CycleBounds could carry the business date. Separately, today_str now holds the business day, so the name misleads.
facility_at re-parses "HH:MM" alongside the existing parse_time_str.

Docs and CI (P3)

docs/guides/statistical_occupancy_models.md:49 still says start + 86400.
CI pins TZ: America/Caracas (ci.yml:14), and 3 mktime-based tests fail under TZ=UTC (test_day_specific_schedule_evaluation, test_holiday_schedule_evaluation, test_business_day_epoch_bounds). CI can't catch a server/facility mismatch outside test_facility_time.py. Fix the fixtures, then run CI in UTC (or both).

Acceptance criteria

Item 1 decided and implemented (or validated in config).
Items 2–11 fixed or explicitly declined.
Full suite green, including under TZ=UTC.

Related: #27, PR #69, #74.

Triage decisions (2026-09-24)

Item 1: validate the config: the calibration window must contain daily_reset_time. Saving a window that doesn't returns VALIDATION_ERROR with the reason. Production (03:30–04:30 around 04:00) passes. Arbitrary windows are not supported.
Item 2: the fresh-deployment default reset becomes "04:00"; that change is made in #74 (single source of defaults). This item just references it.
Items 3–11: fix or explicitly decline, as listed.

> Findings from the round-3 review of PR #69 (comment 1721). Includes one **accepted P2** that needs a design decision, hence `needs-triage`. ## Accepted P2 (needs a decision) 1. **`calibration_cycle_bounds` picks the cycle by calendar day.** A 21:00–22:00 window calibrates the cycle that ended 17 h earlier. A 23:30–00:30 window waits until midnight and then reads the cycle still in progress. Production (03:30–04:30 around 04:00) is unaffected, and a 3-day minute-by-minute simulation found no skipped or doubled calibration there. The constraint is documented in `facility_time.calibration_cycle_bounds`, the config field description and `docs/architecture/admin-data-visualization.md`. **Decide:** support arbitrary windows (choose the most recently *completed* cycle whose end precedes the window), or validate the config so the window must straddle or follow the reset. ## Behaviour (P3) 2. **With the fresh default `daily_reset_time = "00:00"`**, a venue open past midnight (10:00–02:00) is still reported closed after 00:00, and events are stamped `is_working_hours = 0`. The business-day fix only helps when the reset is at or after closing. 3. **A holiday on the calendar day doesn't apply between 00:00 and the reset.** Consistent with the business-day model, but it should be documented. 4. **A pre-reset calibration logs a future `last_calibrated_at`** (`cycle_end − 900`); e.g. a run at 02:00 is logged as 03:45. 5. **Display strings** (`timestamp_formatted`, `peak_time_formatted`) still use server time (`time.localtime`), not facility time. ## Code quality (P3) 6. **The thin wrappers** `get_business_day_epoch_bounds` / `get_completed_business_cycle_bounds` / the new `get_calibration_cycle_bounds`: every caller is a service or a test (the round-2 justification citing AGENTS.md doesn't hold). Their only real use is an argument-order shim for old tests. Inline them or return `CycleBounds`; they currently declare `tuple` returns. 7. **`facility_window_containing` returns a raw pair** read as `window[1]`. Add a small `Window(start, end)`. 8. **`date.fromisoformat(label)` is repeated.** `CycleBounds` could carry the business `date`. Separately, `today_str` now holds the business day, so the name misleads. 9. **`facility_at` re-parses `"HH:MM"`** alongside the existing `parse_time_str`. ## Docs and CI (P3) 10. `docs/guides/statistical_occupancy_models.md:49` still says `start + 86400`. 11. **CI pins `TZ: America/Caracas`** (`ci.yml:14`), and 3 `mktime`-based tests fail under `TZ=UTC` (`test_day_specific_schedule_evaluation`, `test_holiday_schedule_evaluation`, `test_business_day_epoch_bounds`). CI can't catch a server/facility mismatch outside `test_facility_time.py`. Fix the fixtures, then run CI in UTC (or both). ## Acceptance criteria - [ ] Item 1 decided and implemented (or validated in config). - [ ] Items 2–11 fixed or explicitly declined. - [ ] Full suite green, including under `TZ=UTC`. Related: #27, PR #69, #74. ## Triage decisions (2026-09-24) - **Item 1:** validate the config: the calibration window must contain `daily_reset_time`. Saving a window that doesn't returns `VALIDATION_ERROR` with the reason. Production (03:30–04:30 around 04:00) passes. Arbitrary windows are not supported. - **Item 2:** the fresh-deployment default reset becomes `"04:00"`; that change is made in #74 (single source of defaults). This item just references it. - Items 3–11: fix or explicitly decline, as listed.

gabogg added the

needs-triage

label

2026-09-24 22:31:47 +00:00

gabogg referenced this issue

2026-09-24 22:32:33 +00:00

fix(occupancy): business cycles in facility time (#27) #69

gabogg referenced this issue

2026-09-24 22:50:34 +00:00

WIP: feat(research): executive statistics deck (universal zero-scroll, horizon tabs, density scale) #20

gabogg referenced this issue

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