Skip to content

Thread _extract_field_ra_dec through calibration PHASE_DIR reads#14

Merged
jakobtfaber merged 1 commit into
mainfrom
fix-field-phase-dir-shape-rollout
Apr 29, 2026
Merged

Thread _extract_field_ra_dec through calibration PHASE_DIR reads#14
jakobtfaber merged 1 commit into
mainfrom
fix-field-phase-dir-shape-rollout

Conversation

@jakobtfaber
Copy link
Copy Markdown
Contributor

Summary

Rolls PR #8's shape-tolerant FIELD direction helper (_extract_field_ra_dec from dsa110_continuum.calibration.runner) through every remaining raw PHASE_DIR indexing site under dsa110_continuum/calibration/. Fixes the FIELD-shape bug class system-wide for read paths.

Why

The 2026-01-25 hour-22 smoke test (run run_2026-04-29T18_44_03Z.log:256) failed Phase 0 epoch gaincal with:

ERROR Epoch gaincal: FAILED (index 1 is out of bounds for axis 2 with size 1)
      — caller should fall back to static daily G table

The orchestrator's gated fallback meant the smoke test as a whole still completed and produced a coherent DEGRADED verdict — but per-epoch gaincal never ran, leaving a cal_quality.g.phase_scatter_deg=102.5° static table to be applied to the rest of the pipeline. The downstream qa_result=FAIL (catalog completeness 27.4%) is consistent with the degraded calibration.

Root cause is the same FIELD-shape ambiguity PR #8 fixed in runner.py: CASA returns PHASE_DIR as either rows-first (nfields, 1, 2) or column-major (nfields, 2, 1) depending on the table backend. Code doing phase_dir[..., 0, 1] raises on column-major. PR #8 added the helper but only fixed three sites in runner.py; the rest of calibration/ still had raw indexing.

Scope (8 calibration modules + new test + CI line)

File Sites Notes
epoch_gaincal.py _read_ms_phase_center the smoke-test hit site
calibration.py bandpass coherence check RA-only read
bandpass_diagnostics.py per-field phase-center scatter RA + Dec
beam_docker.py EveryBeam pointing extraction single-field by ID
dec_utils.py cal-table strip Dec Dec-only read
preconditions.py LST guard + RA/Dec scatter 2 sites
model.py cached + direct PHASE_DIR reads 3 sites: hoisted RA/Dec arrays out of chunked loop + 2 ms-field-center reads
selfcal.py per-field RA/Dec refactored from pre-slice to helper-then-index

All sites are reads only; _set_field_ra_dec is not needed in this PR. The helper raises ValueError("Unsupported FIELD direction column shape: ...") for unrecognized shapes — strictly more diagnostic than the raw IndexError.

Tests

  • tests/test_epoch_gaincal_field_shape.py (new, 4 tests, cloud-safe via mocked casa_tables.table):
    • test_read_ms_phase_center_rows_first_shape(nfields, 1, 2) historical shape
    • test_read_ms_phase_center_casa_column_major_shape(nfields, 2, 1) (the regression)
    • test_read_ms_phase_center_2d_fallback_shape(nfields, 2) (helper's third supported shape)
    • test_read_ms_phase_center_unsupported_shape_raises — defensive error contract
  • New file added to .github/workflows/python-tests.yml so it runs in cloud CI alongside the existing focused subset.

Verification

$ PYTHONPATH=/workspace pytest tests/test_skymodel_phase_dir.py \
                                tests/test_silent_failures.py \
                                tests/test_epoch_gaincal_field_shape.py -q
18 passed in 104.19s

$ ruff check <8 calibration modules>
26 errors  ← unchanged from origin/main baseline (pre-existing W293/I001/F541/D414)

$ ruff check tests/test_epoch_gaincal_field_shape.py
All checks passed!

$ batch_pipeline.py --date 2026-01-25 --start-hour 22 --end-hour 23 --dry-run
  → identical plan to pre-fix dry-run; orchestrator path unchanged.

Zero new lint debt. Existing pre-existing violations in the touched modules are left untouched per CLAUDE.md hygiene policy.

Out of scope

  • _set_field_ra_dec rollout (no write sites need it in this PR)
  • Removal of legacy dsa110_contimg import shim (separate concern, not bundled)

Expected smoke-test outcome after merge

Re-running the 2026-01-25 hour-22 smoke test should produce pipeline_verdict=CLEAN: Phase 0 epoch gaincal will succeed, the per-epoch G table will apply during Phase 1, and the resulting completeness gate is likely to pass (the prior FAIL was driven by static-G fallback, not by the imaging path itself).

PR #8 added _extract_field_ra_dec / _set_field_ra_dec to runner.py to
abstract over the two CASA table backends' FIELD::PHASE_DIR shapes
(rows-first (nfields, 1, 2) vs CASA column-major (nfields, 2, 1)). It
fixed three call sites in runner.py but the rest of the calibration
package was still doing raw indexing patterns like phase_dir[:, 0, 1],
which raise IndexError on column-major MS — the smoke-test failure
mode observed on 2026-01-25 (run_2026-04-29T18_44_03Z.log line 256:
"Epoch gaincal: FAILED (index 1 is out of bounds for axis 2 with
size 1) — caller should fall back to static daily G table").

This PR rolls _extract_field_ra_dec out to every remaining raw
PHASE_DIR-indexing site under dsa110_continuum/calibration/:

- epoch_gaincal.py: _read_ms_phase_center (the smoke-test hit site)
- calibration.py: phase-coherence check inside the bandpass solver
- bandpass_diagnostics.py: per-field phase-center scatter check
- beam_docker.py: pointing extraction for EveryBeam invocation
- dec_utils.py: median-Dec read for cal-table strip selection
- preconditions.py: LST guard + per-field RA/Dec scatter
- model.py: cached + direct PHASE_DIR reads (3 sites — chunked
  loop hoisted out, two ms-field-center reads in fallback paths)
- selfcal.py: pre-slice refactored to call helper on full array

All sites are reads only; _set_field_ra_dec is not needed in this PR.
The helper raises ValueError("Unsupported FIELD direction column
shape: ...") for unrecognized shapes — a more diagnostic error than
the raw IndexError and exactly the contract the regression test
covers.

Tests:
- tests/test_epoch_gaincal_field_shape.py (new, 4 tests, cloud-safe):
  asserts _read_ms_phase_center handles rows-first, column-major,
  2-D fallback, and raises cleanly on unsupported shape.
- New test added to .github/workflows/python-tests.yml so it runs
  in CI alongside the existing focused subset.

Verification (local, casa6 env):
  pytest tests/test_skymodel_phase_dir.py tests/test_silent_failures.py
        tests/test_epoch_gaincal_field_shape.py -q
  18 passed in 104.19s

  ruff check on all 8 modified calibration modules: 26 errors before,
  26 after — zero new lint debt; pre-existing violations untouched per
  CLAUDE.md hygiene policy. Test file is fully clean.

Out of scope (deferred):
- legacy dsa110_contimg import shim removal (separate concern)
- _set_field_ra_dec rollout (no write sites need it in this PR)

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Copilot AI review requested due to automatic review settings April 29, 2026 20:18
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Rolls out shape-tolerant FIELD phase-center extraction (_extract_field_ra_dec) across calibration read paths to prevent failures (and silent Dec corruption) when CASA returns PHASE_DIR in alternate column-major shapes.

Changes:

  • Replaces raw PHASE_DIR[..., 0, 1]-style indexing with _extract_field_ra_dec across remaining calibration modules.
  • Hoists per-field RA/Dec extraction in model.py to reuse shape-tolerant arrays during chunked processing and MS center reads.
  • Adds a targeted regression test suite for the epoch gaincal PHASE_DIR shape variants and wires it into cloud CI.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
tests/test_epoch_gaincal_field_shape.py Adds regression tests for _read_ms_phase_center covering CASA PHASE_DIR shape variants.
dsa110_continuum/calibration/epoch_gaincal.py Uses _extract_field_ra_dec in _read_ms_phase_center to avoid shape-dependent indexing errors.
dsa110_continuum/calibration/selfcal.py Uses helper on full PHASE_DIR then indexes by field_id to avoid pre-slice ambiguity.
dsa110_continuum/calibration/preconditions.py Updates PHASE_DIR reads to use helper for RA-only and RA/Dec scatter computations.
dsa110_continuum/calibration/model.py Uses helper for per-field RA/Dec extraction and MS field-center reads; reuses arrays in chunk loop.
dsa110_continuum/calibration/dec_utils.py Uses helper to robustly compute median Dec from PHASE_DIR.
dsa110_continuum/calibration/calibration.py Updates coherence-check RA extraction to use helper.
dsa110_continuum/calibration/beam_docker.py Extracts pointing RA/Dec via helper for FIELD table reads.
dsa110_continuum/calibration/bandpass_diagnostics.py Uses helper for per-field RA/Dec extraction in geometric setup checks.
.github/workflows/python-tests.yml Adds the new epoch gaincal PHASE_DIR shape regression test file to the CI subset.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.


ra_deg, dec_deg = epoch_gaincal._read_ms_phase_center("/fake/ms")

assert ra_deg == np.degrees(np.angle(np.mean(np.exp(1j * np.radians([180.0, 181.0]))))) % 360
Copy link

Copilot AI Apr 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In test_read_ms_phase_center_rows_first_shape, the RA assertion uses exact float equality and also applies % 360 inside the np.degrees(...) call. Exact equality can be flaky across platforms, and the modulo should be applied after converting to degrees to match _read_ms_phase_center (otherwise negative angles would produce an incorrect expected value). Consider switching to np.testing.assert_allclose and computing the expected RA with the modulo in degrees.

Suggested change
assert ra_deg == np.degrees(np.angle(np.mean(np.exp(1j * np.radians([180.0, 181.0]))))) % 360
expected_ra = np.degrees(
np.angle(np.mean(np.exp(1j * np.radians([180.0, 181.0]))))
) % 360
np.testing.assert_allclose(ra_deg, expected_ra)

Copilot uses AI. Check for mistakes.
@jakobtfaber jakobtfaber merged commit b0e23d8 into main Apr 29, 2026
5 checks passed
@jakobtfaber jakobtfaber deleted the fix-field-phase-dir-shape-rollout branch April 29, 2026 21:04
jakobtfaber added a commit that referenced this pull request Apr 30, 2026
Use allclose for the PR #14 epoch gaincal RA expectation and keep the test formatted.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants