Take morning darks at the night's science exposure times (#831)

[thusser]

Summary

• Exposes EXPTIME on the archive API: FrameInfo.exptime, an exptime filter/tolerance param on list_frames()/list_options(), and an exptimes key in list_options()'s return, in both PyobsArchive and LocalArchive.
• Adds pyobs.robotic.utils.calibration.science_exptimes_for_night(), deriving the distinct (tolerance-grouped) exptimes a night's science frames used, per instrument/binning. Caches its result at module scope for 5 minutes, keyed by (site, night, tolerance, min_exptime).
• Adds pyobs.utils.exptime_grouping (exptimes_close/group_exptimes), a shared relative-tolerance helper reused by the archive filter and the grouping helper (and intended for Dark masters per exposure time: match science frames by exptime, scale only a reference (600 s) dark #832's per-exptime dark-master grouping later).
• Extends DarkBiasScript with exptimes (explicit list) and match_science_exptimes (derived), running one dark series per exptime, longest-first; the single-exptime default path (including bias, exptime=0) is unchanged. Mutual exclusivity between exptime/exptimes/match_science_exptimes is validated at construction.
• Exptimes below dark_min_exptime (default 5s, per ADR 0015) are dropped before grouping, since calibration treats them as bias-only and never needs a dark master for them.
• ADR 0015-dark-master-strict-exptime-matching-reference-scale-down-only.md flipped from proposed to accepted; plan 2026-09-01-morning-darks-match-science-exptimes.md flipped to implemented.

Scoped to issue #831 only (archive/robotic side). #832 (reduction/Calibration matching+scaling policy) is a separate follow-up plan, not touched here.

estimate_duration() vs. match_science_exptimes

Task.create_script() re-validates a fresh Script (and Archive) instance on every can_run()/estimate_duration() call, so instance-level caching between them doesn't work. Instead, science_exptimes_for_night()'s cache lives at module scope: can_run() (async) warms it, and estimate_duration() (sync, can't query the archive itself) reads it via a new synchronous peek_cached_science_exptimes_for_night(). Falls back to a single-series placeholder estimate if nothing is cached yet (e.g. can_run() hasn't run for that site/night within the cache's 5-minute TTL). can_run() now also surfaces an unreachable archive as a cant-run reason instead of letting that failure surface later during run().

Other known gaps (called out in the plan's acceptance criteria, not solved by this PR)

• PyobsArchive.list_options()'s exptimes key depends on the pyobs-archive server actually returning one — not verifiable from this repo. list_frames() is safe either way: it re-filters by tolerance client-side regardless of server-side EXPTIME support.
• No CHANGELOG entry: the top-of-file version header (v2.1.1.dev1) is already stale relative to the just-released v2.1.1 tag, a pre-existing gap unrelated to this PR — didn't want to guess a version number and add more noise there.

Test plan

• pytest tests/ (excluding integration/xmpp): 1718 passed, 7 pre-existing/unrelated failures in tests/cli/test_pyobsd.py (local-environment pyobs executable lookup, present on develop too).
• ruff check clean.
• pyrefly check (CI's actual invocation, pyobs/ only): 0 errors, same baseline suppressed/warning counts as develop.
• New tests: tests/utils/test_exptime_grouping.py, tests/robotic/utils/test_calibration.py (including cache hit/expiry/clear coverage), plus exptime coverage added to tests/robotic/utils/archive/test_{local,pyobs}_archive.py and tests/robotic/scripts/test_darkbias.py (including the cache-warm-up/fallback split for estimate_duration()).

🤖 Generated with Claude Code

[thusser]

Review

Nice, well-scoped PR — the archive/exptime plumbing, the shared tolerance helper, science_exptimes_for_night(), and the multi-series DarkBiasScript all match the plan and are well tested. I verified: pytest on all touched test files (84 passed), ruff clean, pyrefly check pyobs/ → 0 errors.

A few things to consider before merge:

1. (Medium) DarkBiasScript unions away the per-combo keying science_exptimes_for_night produces.
_resolve_exptimes collapses all (instrument, binning) groups into one global set. On a multi-instrument site, camera A would take darks at exptimes only camera B used — at A's fixed configured binning, no less, so some series can't correspond to any science frame A ever took. The plan even says to log the resolved list per instrument/binning (section 3), which this doesn't do. The script knows its own binning, so it could at least filter by that. If the union is intentional (conservative over-approximation), it should be documented as such.

2. (Medium) Edge semantics for empty/zero exptimes.

• With match_science_exptimes on a night with no qualifying science frames, run() completes having exposed nothing — but only after setting the camera to DARK image type (ImageType.BIAS if exptimes == [0] else ImageType.DARK turns [] into DARK). Silent no-op with camera state left modified; consider failing loudly or taking bias.
• The validator accepts exptimes=[0, 30.0], which runs a 0 s exposure under image type DARK — a "dark" that should really be a bias series — while the plan states bias stays its own single series. Reject 0 inside exptimes or set image type per series.

3. (Low/Med) estimate_duration for match_science_exptimes falls back to exptime=0 → count * 5s.
Documented as a known gap, and I agree a sync method can't run the archive query — but the scheduler plans around this estimate, and ~100 s for a real ~2 h (20×600 s) run is a severe underestimate. A conservative placeholder (e.g. the ADR's 600 s reference) would be safer than 0.

4. (Low) exptimes_close is asymmetric at the tolerance boundary. exptimes_close(100, 101.01) is True, exptimes_close(101.01, 100) is False (tolerance is relative to the second argument). The archive filter always calls it in a consistent direction, so no practical bug, but #832 will reuse this — worth documenting "tolerance relative to b" or making it symmetric.

5. (Low) group_exptimes is a greedy median-drift heuristic, not transitive closure. E.g. group_exptimes([100.0, 100.9, 101.8], 0.01) → [100.45, 101.8] even though 100.9 and 101.8 are pairwise within 1%. The docstring overstates the guarantee; results can shift as values are added. Probably fine for discrete real-world exptimes, but #832's reuse may want union-find grouping.

6. (Low) The PyobsArchive client-side re-filter comment overstates its guarantee. It correctly handles a server that ignores EXPTIME (the tested case), but if the server exact-filters on EXPTIME, near-matches are dropped before the client sees them and the tolerance re-filter has nothing to recover. Harmless for the main path (the helper never passes exptime to list_frames), but "an unsupported or exact-only param doesn't matter" is only true in one direction.

7. (Nit) PyobsArchiveFrameInfoDict declares EXPTIME: float as required while the code uses self.info.get("EXPTIME") — should be NotRequired[float].

8. (Process) Plan section 4's deliverable — documenting the new DarkBiasScript fields in docs/source — is unimplemented even though the plan is marked implemented (the acceptance criteria don't cover it). Also, ADR 0015 flips to accepted while the dark_scale_* policy it records is #832's, which isn't implemented — defensible since this PR consumes dark_min_exptime, but worth an explicit note.

Nits: sum([bool, bool]) in the validator is opaque; the validator messages name only exptimes/match_science_exptimes even though exptime is the third mutually-exclusive mode.

Verdict: approve with minor feedback. Items 1 and 2 I'd want resolved (or explicitly documented as intended) before merge; the rest can be follow-ups.

[thusser]

Addressed in 411a2dc:

1. Union across instrument/binning — fixed. _flatten_matching_exptimes() now narrows to this script's own binning before unioning (instrument still can't be filtered — no INSTRUME-equivalent config field on DarkBiasScript — documented as a known limitation in the docstring). New test: test_only_uses_exptimes_from_its_own_binning.
2. Empty/zero exptime semantics — fixed both: run() now returns before touching IImageType/IExposureTime when _resolve_exptimes() comes back empty (camera left untouched, not silently switched to DARK); exptimes now rejects 0 at validation (bias stays its own single series, per the plan).
3. estimate_duration fallback — now uses _FALLBACK_MATCH_EXPTIME = 600.0 (ADR 0015's reference exptime) instead of the always-0 configured exptime when nothing's cached yet.
4. exptimes_close asymmetry — fixed, now symmetric (tolerance * max(abs(a), abs(b))). New test: test_exptimes_close_is_symmetric.
5. group_exptimes transitivity — docstring now states the greedy, non-transitive behavior explicitly rather than implying a clustering guarantee. Left the algorithm as-is per your own "probably fine for discrete real-world exptimes" call — happy to swap to union-find if Dark masters per exposure time: match science frames by exptime, scale only a reference (600 s) dark #832 needs it.

Full suite still green (1721 passed, same 7 pre-existing/unrelated test_pyobsd.py failures), ruff/pyrefly clean.

[thusser]

Re-review (after e49a955b + 411a2dc7)

Thanks for the quick follow-up — I re-verified the new head (411a2dc7): ruff clean, pyrefly check pyobs/ → 0 errors, touched test files 95 passed / 1 failed (failure is environmental, see A).

Addressed ✅

• Per-combo union (finding 1): _flatten_matching_exptimes() now narrows to the script's own binning and unions across instruments only at that binning, with an honest docstring on why instrument filtering isn't possible (no INSTRUME config field on the script). Warning now names the binning. Covered by test_only_uses_exptimes_from_its_own_binning.
• Empty/zero exptimes (finding 2): run() early-returns before touching image type/exptime when nothing resolves (set_image_type not called — asserted), and the validator rejects 0 in exptimes with a clear "bias is always its own single series" message.
• estimate_duration (finding 3): nice solution — 300 s TTL module-level cache in calibration.py warmed by can_run()'s async query, read by the sync estimate_duration() via peek_cached_science_exptimes_for_night(), cold fallback to _FALLBACK_MATCH_EXPTIME = 600.0 (ADR 0015's reference) instead of the always-0 exptime. Tests cover the warm handoff (fresh instance), cold fallback, TTL expiry, and clear_cache().
• exptimes_close asymmetry (finding 4): now tol * max(abs(a), abs(b)), documented symmetric, tested.
• group_exptimes (finding 5): docstring now honestly documents the greedy median-drift behavior with the exact [100.0, 100.9, 101.8] example.

Still open (minor, from before)

• PyobsArchive's "an unsupported or exact-only param doesn't matter" comment still overstates: with an exact-only server, near-matches are dropped before the client re-filter can recover them. Harmless for the main path (the helper never passes exptime to list_frames).
• PyobsArchiveFrameInfoDict.EXPTIME is typed required but read via .get() — should be NotRequired[float].
• Plan §4's docs deliverable (document the new script fields in docs/source) is still unimplemented while the plan is marked implemented.

New on the follow-up

A. (Should fix before merge) test_can_run_true_with_observer_configured now needs network. It uses a real astroplan.Observer and Time.now().night_obs() → astropy IERS auto-download. Offline (or with a read-only astropy cache, or a clock beyond the bundled IERS coverage — all three here) it fails with ValueError from iers.py. Every other Observer-based test in the repo uses fixed in-coverage times, so the suite previously ran fully standalone. It'll probably pass in CI (network), but consider pinning IERS (conf.auto_max_age = None + bundled file) or mocking Time.now() to keep the suite standalone.

B. (Design note) can_run() is no longer cheap/local for match_science_exptimes. It now executes the full archive query (list_options + per-combo, paginated list_frames for PyobsArchive) to warm the cache — bounded to once per (site, night) per TTL, but the first call after expiry adds real latency to a scheduling-tick method, and an archive outage now reports "cannot run" instead of failing at run time. Deliberate and documented; fine. One robustness gap: _resolve_night() sits outside the try/except, so an IERS failure in night_obs propagates out of can_run() as a hard exception rather than a cannot-run reason — that's exactly the failure mode test A hits here. Consider moving it inside the guarded block.

C. (Nit) Cache key omits archive identity — (site, night, tolerance, min_exptime) only; two archives serving the same site/night in one process would collide for up to 5 min. Acknowledged in the code comment; unlikely in practice.

D. (Nit) Estimate depends on call order — real numbers only if can_run() ran first within the TTL; cold calls get the 600 s placeholder (sensible, and tested).

Verdict

All substantive findings resolved with good tests. Ship after A (make the observer test IERS-independent); B–D and the three "still open" items can be follow-ups.