Per-exposure-time dark masters, reference-master scale-down only (#832)

[thusser]

Summary

• Reduction._create_master_darks() groups a night's raw DARK frames by exptime (tolerance-grouped, reusing Take morning darks at the exposure times used for science frames during the night #831's pyobs.utils.exptime_grouping) and creates one master per group instead of a single night-wide dark, longest-first. Frame groups with fewer than 3 members are skipped individually with a warning rather than aborting dark reduction for the whole instrument/binning.
• FILENAME gains an exptime component ({EXPTIME|exptime}), shared by BIAS/SKYFLAT too since it's one pattern — via a new FilenameFormatter "exptime" function that renders 600.0 as 600. MasterCalibCreated carries the dark's exptime. ReductionBase's master-frame cache key gains exptime (None for non-DARK).
• Pipeline.find_master() gains exptime/exptime_tolerance/exptime_max params: for DARK, an exptime match ranks ahead of a merely time-close non-match, and exptime_max enforces scale-down-only when searching for a reference master.
• Calibration implements ADR 0015-dark-master-strict-exptime-matching-reference-scale-down-only.md's policy via a new _find_dark_master(), checked in order against the science frame's EXPTIME: (1) exact match within dark_exptime_tolerance → unscaled; (2) no exact match and EXPTIME < dark_min_exptime → bias-only, not an error; (3) no exact match, above the minimum, and a reference master (dark_scale_exptime, default 600s) exists → scaled down to the science exptime; (4) allow_unmatched_dark_scale=True → today's always-scale-whatever's-nearest behavior; (5) otherwise → ValueError, caught the same way as any other missing-master case in __call__.
• _CalibrationCache now keys DARK entries by the target exptime searched for (science exptime for an exact-match lookup, or the reference exptime), not the science image's own EXPTIME header — a science frame's own exptime no longer identifies which master it needs once scaling is in play.
• _CCDDataCalibrator gains a dark_scale: bool param threaded from Calibration.__call__, so an exact-match dark isn't rescaled by ccdproc.ccd_process().
• ADR 0015 was already accepted (flipped in Take morning darks at the night's science exposure times (#831) #840); this plan (2026-09-01-per-exptime-dark-masters.md) is flipped proposed → implemented, both open questions resolved (relative-only tolerance; hard filename-pattern rename, documented rather than dual-pattern).

Scoped to issue #832 only (reduction/pipeline side). #831 (archive/robotic side) already landed on develop via PR #840.

Design notes / judgment calls

• _verify_image_header now also requires EXPTIME. It was already an implicit requirement of _CCDDataCalibrator (data_exposure=...EXPTIME...); checking it upfront turns a bare KeyError mid-calibration into the same ValueError path every other missing-header case already takes.
• The bias-only branch (2) needed no new _CCDDataCalibrator path. dark: Image | None = None was already handled end-to-end (ccd_process(dark_frame=None, ...)); the plan's open question here is resolved by reuse, not a new branch.
• Cache key considered and rejected: full archive/image config dump. First pass keyed _CalibrationCache by deriving exptime from the master image's own header — works, but doesn't generalize past this PR. Landed on keying explicitly by the target exptime the caller searched for, passed in by _find_dark_at, which is what actually identifies "which master do I need" once scaling means the returned master's own exptime can legitimately differ from what was asked for.

Test plan

• pytest tests/ (excluding integration/xmpp): 1758 passed, same 7 pre-existing/unrelated tests/cli/test_pyobsd.py failures as develop (local-environment pyobs executable lookup).
• ruff check . clean.
• black --check clean.
• pyrefly check pyobs/ (CI's actual invocation): 0 errors, same baseline suppressed/warning counts as develop.
• New/updated tests: tests/utils/pipeline/test_pipeline.py (new — Pipeline.find_master's exptime ranking/exptime_max), tests/utils/pipeline/test_reduction.py (per-exptime dark grouping, under-populated-group skip, no-bias skip), tests/images/processors/misc/test_calibration.py (all 5 _find_dark_master policy branches + _CCDDataCalibrator.dark_scale), tests/images/processors/misc/test_calibration_cache.py (exptime keying), tests/utils/test_exptime_grouping.py (new group_by_exptime).

🤖 Generated with Claude Code

[thusser]

Review — approved with minor changes

Reviewed the full diff (33df9156 vs 7e3a6edb), ran the affected tests, lint/type checks, and an end-to-end Reduction → Archive → Calibration roundtrip. The design faithfully implements ADR 0015 and the tests cover all five _find_dark_master branches. Details below, ordered by severity.

Verified

• Changed-area tests: 55/55 pass; tests/utils/pipeline/ + tests/images/processors/ = 142/142 pass.
• ruff check, black --check, pyrefly check pyobs/ clean.
• Full suite: 1735 passed / 21 failed — all failures (fitsheader, dummy telescope) reproduce at the base commit; environmental, not PR-caused.
• E2E: darks at 45s/600s → distinct masters (…-dark-45-1x1.fits, …-dark-600-1x1.fits); 45s/600s science matches exactly unscaled; 900s science correctly rejected (no upward scaling).

1. Bug — uncaught KeyError when a master dark lacks EXPTIME (fix before merge)

calibration.py:359-363, exact-match path:

if exptime_max is None and not exptimes_close(
    float(master.header["EXPTIME"]), target_exptime, self._dark_exptime_tolerance
):

A master whose header has no EXPTIME (exactly what a legacy pre-#831 master is — FrameInfo.exptime is None precisely when the header lacks the keyword) raises KeyError. __call__ catches only ValueError (line 223), so it propagates and the frame fails hard — the same bare-KeyError failure the _verify_image_header change claims to eliminate, just moved from the science frame to the master. Reproduced locally.

Suggested fix (missing → "not an exact match", falling through to bias-only/reference/strict as intended):

if exptime_max is None and (
    "EXPTIME" not in master.header
    or not exptimes_close(float(master.header["EXPTIME"]), target_exptime, self._dark_exptime_tolerance)
):
    return None

Plus a test with a master lacking EXPTIME.

2. Changelog entry missing (ADR requirement)

ADR 0015 explicitly says the strict-by-default behavior change "must be called out in the changelog, not discovered from a support ticket"; the plan's acceptance criteria require a "changelog-visible migration step". CHANGELOG.rst has no entry. Relatedly, the PR/plan wording "existing Calibration configs keep working unchanged" isn't quite accurate: with the new defaults, a site with only a 600s master and science at 900s goes from silent scale-up to warning + no dark correction. That's the ADR's deliberate choice — but it's a behavior change, worth a changelog note.

3. DARK cache never hits when science frames carry FILTER (low-medium, pre-existing but amplified)

_CalibrationCache._get_image_cache_keys includes FILTER for all types; the add is keyed off the master's header, the get off the science image's. Master darks typically lack FILTER while science frames carry e.g. g → cache miss on every frame, now two archive queries each (exact + reference). Pre-existing for the old path, but this PR rewrote the lookup; the test fixtures avoid it by omitting FILTER from both images. _find_master_in_archive already ignores filter for DARK — the cache key should too (_get_cache_keys: drop FILTER for ImageType.DARK).

4. All darks lacking EXPTIME → no dark master at all (low)

reduction.py:208-212 drops every frame with exptime=None and returns [] if none remain; previously such nights still produced a single master (rescaled at calibration). For legacy instruments this silently disables dark calibration. Consider falling back to one legacy group when all darks lack EXPTIME (skip only when mixed).

Nits

• Error message says "no reference master <= 600.0s" but the guard/ceiling actually allow up to 600*(1+tolerance); ADR also specifies listing the available master exptimes, which the message doesn't.
• Tolerance band can scale ~2% up in a pathological corner (science at 600*1.01, only a 600*0.99 master exists) — fine in practice, a comment acknowledging the band would help.

Verdict: sound design, good tests, faithful to ADR 0015. Please fix #1 (one-line guard + test) and #2 (changelog entry) before merge; #3/#4 recommended follow-ups.

[thusser]

Re-review of fix commit 1ce95eca — good, one residual issue

Thanks — the fix commit cleanly addresses all four findings from the first review:

• ✅ KeyError in _find_dark_at — fixed exactly as suggested ("EXPTIME" not in master.header short-circuit) + regression test with a legacy master.
• ✅ Changelog — thorough CHANGELOG.rst entry with the upgrade-check guidance; plan wording corrected ("configs keep validating … behavior changes").
• ✅ DARK/BIAS cache filter — _get_cache_keys now nulls filter_name for BIAS/DARK, matching _find_master_in_archive; tests cover the hit (BIAS/DARK) and the still-miss (SKYFLAT) cases.
• ✅ Legacy all-no-EXPTIME darks — falls back to one untagged master instead of zero, with the "unknown" filename placeholder and exptime=None in cache key/progress event.
• Bonus: the LocalArchive NaN→None normalization is a real catch (pandas float64 coercion), well-tested.

Verified on 1ce95eca: all affected test files pass (89 tests), ruff/black/pyrefly clean.

Residual issue (medium) — legacy untagged master + allow_unmatched_dark_scale=True still crashes with an uncaught KeyError

The new legacy fallback in _create_master_darks produces a master with no EXPTIME header, and the changelog tells legacy sites to set allow_unmatched_dark_scale=True as the migration path. On that exact path:

1. _find_dark_master branch 4 returns (untagged_master, True) (calibration.py:297-300).
2. _CCDDataCalibrator.__init__ unconditionally does self._dark_exp_time = dark.header["EXPTIME"] (_ccddata_calibrator.py:53) → KeyError.
3. Calibration.__call__ catches only ValueError → the KeyError propagates and every science frame fails hard.

Reproduced end-to-end (archive with only the untagged legacy master, require_bias=False, require_flat=False, allow_unmatched_dark_scale=True):

CALIBRATION CRASHED: KeyError: "Keyword 'EXPTIME' not found."

Note it also raises with dark_scale=False, since the header read is unconditional. Suggested fix:

# _ccddata_calibrator.py — only read EXPTIME when it will actually be used
if dark is not None and dark_scale:
    self._dark_exp_time = dark.header["EXPTIME"]

and in branch 4, don't ask ccdproc to scale a master whose exptime is unknown:

fallback = await self._find_master(image, ImageType.DARK, max_days=self._max_days_dark)
return fallback, "EXPTIME" in fallback.header

A test calibrating a science frame against an untagged legacy master with allow_unmatched_dark_scale=True would lock this in.

Nits

• Branch-3 comment says "Never scales up in practice" — not strictly true: with a single 45s master and default dark_scale_exptime=600, science at 100s goes exact-miss → reference lookup → 45s master (closest to 600, within the 606s ceiling) → scaled up 2.2×. Config-dependent corner (the changelog steers such sites to allow_unmatched_dark_scale=True), but either enforce master.exptime >= science_exptime on the reference result or soften the comment.
• _available_dark_exptimes scans the whole archive (no max_days window) on every strict-failure frame; fine for typical DARK-master counts, but a time window or cap would be safer on large archives.

Verdict: all original findings resolved correctly; the remaining KeyError (this re-review's finding 1) is the same class of bug the PR set out to eliminate and sits on the exact migration path the changelog now recommends — worth fixing before merge.