feat: consume instrument_capabilities in 4 leaf scripts (§A.8) - #867
Merged
Conversation
ImagingScript, PointingScript, DarkBiasScript, and AutoFocusScript now read data.instrument_capabilities and use real portal-declared numbers wherever available, falling back to today's flat fudge constants whenever the lookup misses at any level (no data, no capabilities, no matching module_name row, or the specific field not set on the matched row): - ImagingScript: adds real per-binning readout time (previously not counted at all) and real per-wheel filter-change time (also previously absent, now counted once per actual filter transition across the full instrument_configs * repeats sequence -- a config with no optical_filter set never counts as a transition, since there's no way to know whether that's actually a change). Telescope slew replaces the flat 60.0 fudge. - PointingScript / AutoFocusScript: telescope slew replaces their flat 60.0 fudges. - DarkBiasScript: camera's matching BinningOption.readout_time_s replaces the flat 5.0s readout fudge. - SelectorScript unchanged, per the plan's Non-goals (unclear which capability field, if any, actually fits "mode change" duration). New TelescopeCapability.estimate_slew_time_s() (pyobs/robotic/instruments.py) centralizes the "typical slew distance / real rate" placeholder shared by all three telescope-slewing scripts, rather than duplicating the constant and the None-rate fallback three times. §A.8 of specs/plans/2026-09-01-instrument-capability-duration-estimates.md -- the last piece of §A. §A.4 (PortalTaskArchive's concrete fetch/cache, which needs a new pyobs-portal marker endpoint that doesn't exist yet) is the one remaining slice; every backend still returns None today, so this PR changes no runtime behavior by itself until real capability data is actually wired up to reach these scripts. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019XoNCe7YyJELc8xup1zVdE
thusser
commented
Sep 3, 2026
thusser
left a comment
Member
Author
There was a problem hiding this comment.
Review: approve — the §A.8 consumption layer is well-built
Every fallback preserves today's exact formulas, the capability lookups match the model layer's semantics, and the new filter-transition logic is careful and well-tested. No blockers.
Verified
- Fallback equivalence holds. With no capabilities (the production state today — every backend still returns
None), all four scripts reproduce their old formulas exactly: imaging's readout → 0.0 (it was never counted), slew → flat 60.0, acquisition → +30.0, no filter time; darkbias'sreadout = 5.0in all three branches (exptimes list, match-science, single-exptime); pointing/autofocus → 60.0. So the "no runtime behavior change until data is wired" claim is accurate. - Lookup semantics are consistent. Camera/telescope/filter-wheel lookups key on the scripts' existing module-name fields; readout only applies when the exact
(x, y)binning is declared on the portal row (conservative — no nearest-binning guessing);_readout_time_sreturns 0.0 (not a fudge) for imaging since imaging previously counted no readout, while darkbias keeps its 5.0 — matching the plan's per-script fallback semantics. - Filter-change counting is correct. Transitions are counted per config pair (not per exposure — a
count-long series shares one filter), the sequence wraps across the repeat boundary,None(use-current-filter) never counts, and the unknown starting filter isn't counted. Test cases cover R,V,V,R→2, R,V|R,V→3 across repeats, and None-touching→0. estimate_slew_time_s()guardsNone/non-positive rates, and the 90° placeholder is documented with plan + #858/#859 references.- Tooling: 66/66 tests on the 5 touched files pass, full
tests/robotic/scripts/139/139 pass,ruff/blackclean. pyrefly's 30 errors are all intest_autofocus.py/test_darkbias.pyon pre-existing lines — I verified the identical error set exists ondevelop(exact line-shift mapping: uniform +1/+2 from the new imports, +27 past the appended test blocks). The "20 new tests" claim checks out (3+2+9+3+3).
Notes (non-blocking)
- Design: estimation policy now lives on a data-mirror model.
_TYPICAL_SLEW_DEG+estimate_slew_time_s()put a pyobs-side planning decision (the placeholder 90° distance) onTelescopeCapability, which #864 deliberately kept a pure serializer mirror. It's self-contained and convenient, but when #858/#859 replace the placeholder with real distances — which will likely differ per script (pointing vs. autofocus slews) — a shared helper parameterized by distance or per-script constants would keep the model clean. - Imaging: the slew term applies regardless of whether the config actually slews. A pure bias/dark
instrument_configssequence still adds slew time (rate-based now, 60.0 flat before) — pre-existing conceptual fudge (the +60 was unconditional), just noting the estimate now distinguishes rate but not whether a slew happens at all. Also, each filter transition costs onefilter_change_time_s(portal's documented one-position-step estimate) regardless of how many wheel positions a change actually spans. Both are reasonable first passes per the plan; worth a sentence in the docstring. - Nit: imaging.py's new
from pyobs.robotic.instruments import CameraCapabilityis a runtime import used only in an annotation — the file's own convention (TaskData) isTYPE_CHECKING; moving it would be consistent (negligible today sinceinstruments.pyis import-light).
Summary
Faithful to §A.8, careful fallback design, and the strongest test coverage of the three PRs in this series. Merge as-is; note 1 is the one to keep in mind for the #858/#859 follow-ups.
- TelescopeCapability.estimate_slew_time_s() takes distance_deg (default DEFAULT_SLEW_DISTANCE_DEG, same 90.0 every caller uses today) instead of a hardcoded module constant -- #858/#859's real distances will likely differ per script, so the model stays a pure portal mirror rather than baking in a shared policy decision - ImagingScript.estimate_duration() docstring notes two carried-over simplifications: slew time is added unconditionally even for a bias/dark-only sequence, and each filter transition costs one flat filter_change_time_s regardless of how many positions it spans - moved the CameraCapability import to TYPE_CHECKING in imaging.py, matching the file's existing TaskData convention (only used in an annotation) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019XoNCe7YyJELc8xup1zVdE
4 tasks
thusser
added a commit
that referenced
this pull request
Sep 3, 2026
…emented §A landed across pyobs-core#864/#865/#867/#868, released as pyobs-core v2.4.0. §B landed in pyobs-portal#144/#145 (dependency bumped, real path now exercised end-to-end, 0 skipped). All test-plan items checked off except the manual script-builder browser click-through, left open -- automated coverage exercises the same code path but nobody's watched it in a browser yet. specs/plans/index.md's entry marked implemented; fleet-open-items.md's plan entries removed per its own maintenance rule (implemented items don't stay, they get removed outright), #858/#859's dangling "plan below" reference reworded since the plan itself is no longer listed there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019XoNCe7YyJELc8xup1zVdE
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
§A.8 of
specs/plans/2026-09-01-instrument-capability-duration-estimates.md— the last piece of §A. Four scripts now readdata.instrument_capabilitiesand use real portal-declared numbers wherever available, falling back to today's flat fudge constants whenever the lookup misses at any level (nodata, no capabilities, no matchingmodule_namerow, or the specific field not set on the matched row):ImagingScript: adds real per-binning readout time (previously not counted at all) and real per-wheel filter-change time (also previously absent — now counted once per actual filter transition across the fullinstrument_configs * repeatssequence; a config with nooptical_filterset never counts as a transition, since there's no way to know whether that's actually a change). Telescope slew replaces the flat60.0fudge.PointingScript/AutoFocusScript: telescope slew replaces their flat60.0fudges.DarkBiasScript: camera's matchingBinningOption.readout_time_sreplaces the flat5.0s readout fudge.SelectorScript: unchanged, per the plan's Non-goals (unclear which capability field, if any, actually fits "mode change" duration).New
TelescopeCapability.estimate_slew_time_s()(pyobs/robotic/instruments.py) centralizes the "typical slew distance / real rate" placeholder shared by all three telescope-slewing scripts, instead of duplicating the constant and the None-rate fallback three times.Not in this PR
§A.4 (
PortalTaskArchive's concrete fetch/cache) — needs a new pyobs-portal marker endpoint that doesn't exist yet, tracked as its own follow-up. Every backend still returnsNoneforget_instrument_capabilities()today, so this PR changes no runtime behavior until real capability data is actually wired up to reach these scripts.Test plan
test_instruments.py(TelescopeCapability.estimate_slew_time_s()) and all 4 scripts' test files (real-value path, missing-module fallback, missing-field fallback, filter-change transition counting including the "None never counts as a change" and cross-repeat-boundary cases)pytest tests/(excludingintegration/xmpp) — 1886 passed (up from 1866), no regressionsruff check/black --check/pyrefly check— clean (30 pre-existing, unrelated pyrefly errors on 2 test files confirmed identical ondevelop)🤖 Generated with Claude Code
https://claude.ai/code/session_019XoNCe7YyJELc8xup1zVdE