Skip to content

Commit 5ef6eea

Browse files
Claudeclaude
andcommitted
fix: regenerate datasets left on disk by a small-datasets run
should_simulate() was existence-only and asymmetric. It force-regenerated when PYAUTO_SMALL_DATASETS=1, but had no corresponding check on the full-resolution path, so a dataset written by an earlier capped run was reused indefinitely. Because dataset/ is gitignored in the workspaces, CI clones fresh and always simulates, and so can never hit this. Locally the directory persists, and since PYAUTO_SMALL_DATASETS=1 is the default for most harness runs, a single earlier run leaves 16x16 FITS that every later full-resolution run then loads silently. The result is deterministic, bit-identical assertion failures that pass in CI on the same commit (autolens_workspace_test#260) — an active trap, since the failures look exactly like fresh correctness regressions. The regime is recorded nowhere on disk, so it is inferred from data.fits: the cap in Mask2D.circular / Grid2D.uniform rewrites anything larger to exactly SMALL_DATASETS_SHAPE_NATIVE, so a data.fits at exactly (16, 16) can only have come from a capped run. The predicate ends in shutil.rmtree, so it is deliberately narrow: - exactly the cap shape, never "at or below" — the cap cannot emit 12x12, so widening the test buys no detection and only risks real data; - data.fits by name, never "the first FITS in the directory" — PSF kernels are legitimately tiny at full resolution (11x11 is common, and one on disk is already byte-identical in size to a capped data.fits), so a glob would regenerate every dataset carrying one on every run; - unknown means no — a missing, unreadable or non-2D data.fits preserves the existing existence-only behaviour rather than deleting. Scope: this covers the imaging manifestation only. Point-source and weak-lensing datasets are JSON with no FITS, and interferometer datasets keep their shape under the cap while their values change, so both regress to existence-only and remain exposed. Closing those needs the regime recorded at write time rather than inferred at read time; filed separately. Cost is one FITS header read (~0.6ms warm), and only when a data.fits exists. No re-simulation in the steady state. Tests cover all four regime transitions — the bug was precisely that one of the four was never exercised — plus the false-positive guards, every one of which asserts a dataset is PRESERVED. Control-tested: the small->full regression test returns False against the unfixed body. Full suite 1168 passed. Verified end to end against the original failure: poison the dataset with one PYAUTO_SMALL_DATASETS=1 run, then run imaging/jax_grad/lp.py under the full_datasets profile. Before: AssertionError, source gradients ~1e-12. After: regenerated at full resolution, checks pass. Co-Authored-By: Claude <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VEHLT33XpVcRt5YCJGLRMJ
1 parent 2784056 commit 5ef6eea

2 files changed

Lines changed: 283 additions & 4 deletions

File tree

autoarray/util/dataset_util.py

Lines changed: 100 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -69,25 +69,121 @@ def cap_array_2d_for_small_datasets(array_2d, pixel_scales):
6969
)
7070

7171

72+
def _on_disk_shape_native(data_path):
73+
"""
74+
Returns the ``(rows, columns)`` shape of the first 2D image in the FITS file
75+
at ``data_path``, or ``None`` if that cannot be determined.
76+
77+
Only the headers are read, never the pixel data, so this costs a single
78+
small read regardless of dataset size.
79+
80+
``None`` means "unknown", and every caller must treat it as "leave the
81+
dataset alone" — this function feeds a destructive predicate, so an
82+
unreadable or unconventional file must never be grounds for deleting it.
83+
"""
84+
from astropy.io import fits
85+
86+
try:
87+
with fits.open(data_path) as hdu_list:
88+
for hdu in hdu_list:
89+
header = hdu.header
90+
if header.get("NAXIS") == 2:
91+
# NAXIS1 is the fastest-varying axis (columns), NAXIS2 the
92+
# rows, so the numpy-order shape is (NAXIS2, NAXIS1).
93+
return (header["NAXIS2"], header["NAXIS1"])
94+
except Exception:
95+
return None
96+
97+
return None
98+
99+
100+
def _is_small_datasets_on_disk(dataset_path):
101+
"""
102+
Returns True if the dataset on disk at ``dataset_path`` was written by a
103+
simulator running under ``PYAUTO_SMALL_DATASETS=1``.
104+
105+
The regime is not recorded anywhere on disk, so it is inferred from the
106+
shape of ``data.fits``: the cap in ``Mask2D.circular`` / ``Grid2D.uniform``
107+
rewrites anything larger than ``SMALL_DATASETS_SHAPE_NATIVE`` to *exactly*
108+
that shape, so an on-disk ``data.fits`` at exactly (16, 16) can only have
109+
come from a capped run.
110+
111+
Three deliberate narrownesses, all of them because this predicate ends in
112+
``shutil.rmtree`` and a false positive silently deletes a user's data:
113+
114+
- **Exactly** the cap shape, never "at or below" it. The cap cannot emit
115+
12x12, so widening the test buys no detection and only adds risk.
116+
- **``data.fits`` by name**, never "the first FITS in the directory". PSF
117+
kernels are legitimately tiny at full resolution (11x11 is common), and a
118+
glob would regenerate every dataset carrying one on every single run.
119+
- **Unknown means no.** A missing, unreadable or non-2D ``data.fits``
120+
returns False, preserving the existence-only behaviour for the dataset
121+
families this cannot speak about (see the caveat in ``should_simulate``).
122+
"""
123+
data_path = Path(dataset_path) / "data.fits"
124+
125+
if not data_path.exists():
126+
return False
127+
128+
return _on_disk_shape_native(data_path) == SMALL_DATASETS_SHAPE_NATIVE
129+
130+
72131
def should_simulate(dataset_path):
73132
"""
74133
Returns True if the dataset at ``dataset_path`` needs to be simulated.
75134
76-
When ``PYAUTO_SMALL_DATASETS=1`` is active, any existing dataset
77-
is deleted so the simulator re-creates it at the reduced resolution. This
78-
avoids shape mismatches between full-resolution FITS files on disk and the
79-
15x15 mask/grid cap applied by the env var.
135+
A dataset is invalid when it was simulated under a different resolution
136+
regime than the one in force now, because ``PYAUTO_SMALL_DATASETS=1`` caps
137+
masks and grids to ``SMALL_DATASETS_SHAPE_NATIVE``. Both directions are
138+
checked:
139+
140+
- Entering the **small** regime, any existing dataset is deleted so the
141+
simulator re-creates it at the reduced resolution, avoiding shape
142+
mismatches between full-resolution FITS on disk and the capped
143+
mask/grid.
144+
- Entering the **full** regime, a dataset left behind by an earlier capped
145+
run is likewise deleted. Existence alone cannot distinguish the two, so
146+
the regime is inferred from the data on disk
147+
(``_is_small_datasets_on_disk``).
148+
149+
That second check is what makes a local FAIL mean something. ``dataset/``
150+
is gitignored in the workspaces, so CI clones fresh and always simulates,
151+
while a local checkout keeps its dataset indefinitely — and since
152+
``PYAUTO_SMALL_DATASETS=1`` is the default for most harness runs, a single
153+
earlier run would leave capped FITS that every later full-resolution run
154+
then loaded silently, producing deterministic, environment-only failures
155+
that could not be reproduced in CI (autolens_workspace_test#260).
80156
81157
Use this as a drop-in replacement for ``not path.exists(dataset_path)`` in
82158
the workspace auto-simulation pattern::
83159
84160
if aa.util.dataset.should_simulate(dataset_path):
85161
subprocess.run([sys.executable, "scripts/.../simulator.py"], check=True)
162+
163+
Known gap
164+
---------
165+
The full-regime check reads ``data.fits``, so it covers imaging-style
166+
datasets only. It cannot see a stale capped dataset whose corruption is not
167+
visible in that file's shape:
168+
169+
- point-source and weak-lensing datasets, which are JSON with no FITS;
170+
- interferometer datasets, whose visibility count is fixed by the uv file
171+
while the real-space grid behind it is capped, so the capped and full
172+
files share a shape and differ only in values.
173+
174+
Those regress to the previous existence-only behaviour rather than being
175+
fixed here. Closing them needs the regime recorded at write time rather
176+
than inferred at read time.
86177
"""
87178
if os.environ.get("PYAUTO_SMALL_DATASETS") == "1":
88179
if Path(dataset_path).exists():
89180
shutil.rmtree(dataset_path)
90181

182+
return not Path(dataset_path).exists()
183+
184+
if Path(dataset_path).exists() and _is_small_datasets_on_disk(dataset_path):
185+
shutil.rmtree(dataset_path)
186+
91187
return not Path(dataset_path).exists()
92188

93189
SMALL_DATASETS_N_CATALOGUE = 25

test_autoarray/util/test_dataset_util.py

Lines changed: 183 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,3 +90,186 @@ def test__env_set__non_square_above_cap__center_crops_to_16x16(monkeypatch):
9090

9191
assert result.shape_native == SMALL_DATASETS_SHAPE_NATIVE
9292
assert pixel_scales == SMALL_DATASETS_PIXEL_SCALES
93+
94+
95+
"""
96+
__should_simulate — regime transitions__
97+
98+
`should_simulate` must regenerate a dataset whenever the resolution regime on
99+
disk differs from the one in force. Before autolens_workspace_test#260 only the
100+
full->small transition was implemented; small->full silently reused capped FITS
101+
at full resolution, producing deterministic failures that no CI run could
102+
reproduce (CI clones fresh, so it never has a stale dataset).
103+
104+
All four transitions are covered below because the bug was precisely that one
105+
of the four was never exercised.
106+
"""
107+
108+
import json
109+
110+
from autoarray.util.dataset_util import (
111+
should_simulate,
112+
_is_small_datasets_on_disk,
113+
_on_disk_shape_native,
114+
)
115+
116+
def _write_dataset(dataset_path, shape, extra_files=()):
117+
"""Write a minimal dataset directory containing a `data.fits` of `shape`."""
118+
dataset_path.mkdir(parents=True, exist_ok=True)
119+
120+
aa.output_to_fits(
121+
values=np.ones(shape),
122+
file_path=str(dataset_path / "data.fits"),
123+
overwrite=True,
124+
)
125+
126+
for name, file_shape in extra_files:
127+
aa.output_to_fits(
128+
values=np.ones(file_shape),
129+
file_path=str(dataset_path / name),
130+
overwrite=True,
131+
)
132+
133+
return dataset_path
134+
135+
136+
def test__small_regime__existing_full_dataset__is_deleted_and_resimulated(
137+
monkeypatch, tmp_path
138+
):
139+
monkeypatch.setenv("PYAUTO_SMALL_DATASETS", "1")
140+
dataset_path = _write_dataset(tmp_path / "dataset", (180, 180))
141+
142+
assert should_simulate(str(dataset_path)) is True
143+
assert not dataset_path.exists()
144+
145+
146+
def test__small_regime__existing_small_dataset__is_still_deleted_and_resimulated(
147+
monkeypatch, tmp_path
148+
):
149+
# The small path is unconditional by design: it cannot know the capped
150+
# dataset on disk was produced by the SAME cap, so it always regenerates.
151+
monkeypatch.setenv("PYAUTO_SMALL_DATASETS", "1")
152+
dataset_path = _write_dataset(tmp_path / "dataset", SMALL_DATASETS_SHAPE_NATIVE)
153+
154+
assert should_simulate(str(dataset_path)) is True
155+
assert not dataset_path.exists()
156+
157+
158+
def test__full_regime__stale_small_dataset__is_deleted_and_resimulated(
159+
monkeypatch, tmp_path
160+
):
161+
# THE REGRESSION TEST. Before the fix this returned False and the capped
162+
# FITS were loaded at full resolution.
163+
monkeypatch.delenv("PYAUTO_SMALL_DATASETS", raising=False)
164+
dataset_path = _write_dataset(tmp_path / "dataset", SMALL_DATASETS_SHAPE_NATIVE)
165+
166+
assert should_simulate(str(dataset_path)) is True
167+
assert not dataset_path.exists()
168+
169+
170+
def test__full_regime__full_dataset__is_kept(monkeypatch, tmp_path):
171+
monkeypatch.delenv("PYAUTO_SMALL_DATASETS", raising=False)
172+
dataset_path = _write_dataset(tmp_path / "dataset", (180, 180))
173+
174+
assert should_simulate(str(dataset_path)) is False
175+
assert (dataset_path / "data.fits").exists()
176+
177+
178+
def test__full_regime__absent_dataset__simulates(monkeypatch, tmp_path):
179+
monkeypatch.delenv("PYAUTO_SMALL_DATASETS", raising=False)
180+
181+
assert should_simulate(str(tmp_path / "does_not_exist")) is True
182+
183+
184+
"""
185+
__should_simulate — false-positive guards__
186+
187+
The full-regime branch ends in `shutil.rmtree`, so every one of these asserts
188+
that a dataset is PRESERVED. A regression here silently deletes real data.
189+
"""
190+
191+
192+
def test__full_regime__tiny_psf_alongside_full_data__is_kept(monkeypatch, tmp_path):
193+
# PSF kernels are legitimately tiny at full resolution (11x11 is the common
194+
# workspace value, and a 16x16 PSF is a plausible one). The check must key
195+
# on `data.fits` by name — a "first FITS in the directory" implementation
196+
# would delete this dataset on every run forever.
197+
monkeypatch.delenv("PYAUTO_SMALL_DATASETS", raising=False)
198+
dataset_path = _write_dataset(
199+
tmp_path / "dataset",
200+
(180, 180),
201+
extra_files=(("psf.fits", (11, 11)), ("noise_map.fits", (180, 180))),
202+
)
203+
204+
assert should_simulate(str(dataset_path)) is False
205+
assert (dataset_path / "data.fits").exists()
206+
assert (dataset_path / "psf.fits").exists()
207+
208+
209+
def test__full_regime__psf_at_exactly_the_cap_shape__does_not_trigger_deletion(
210+
monkeypatch, tmp_path
211+
):
212+
monkeypatch.delenv("PYAUTO_SMALL_DATASETS", raising=False)
213+
dataset_path = _write_dataset(
214+
tmp_path / "dataset",
215+
(180, 180),
216+
extra_files=(("psf.fits", SMALL_DATASETS_SHAPE_NATIVE),),
217+
)
218+
219+
assert should_simulate(str(dataset_path)) is False
220+
assert (dataset_path / "data.fits").exists()
221+
222+
223+
def test__full_regime__below_cap_data__is_kept_because_the_cap_emits_exactly_16x16(
224+
monkeypatch, tmp_path
225+
):
226+
# The cap rewrites anything larger to EXACTLY (16, 16) and never produces
227+
# 12x12, so a 12x12 dataset was not capped and must be left alone. This is
228+
# why the predicate is `==` and not `<=`.
229+
monkeypatch.delenv("PYAUTO_SMALL_DATASETS", raising=False)
230+
dataset_path = _write_dataset(tmp_path / "dataset", (12, 12))
231+
232+
assert should_simulate(str(dataset_path)) is False
233+
assert (dataset_path / "data.fits").exists()
234+
235+
236+
def test__full_regime__json_only_dataset__is_kept(monkeypatch, tmp_path):
237+
# Point-source and weak-lensing datasets carry no FITS at all. The check
238+
# cannot speak about them, so it must fall back to existence-only rather
239+
# than delete. (These remain exposed to the underlying bug — see the
240+
# "Known gap" section of should_simulate's docstring.)
241+
monkeypatch.delenv("PYAUTO_SMALL_DATASETS", raising=False)
242+
dataset_path = tmp_path / "dataset"
243+
dataset_path.mkdir()
244+
(dataset_path / "point_dataset.json").write_text(json.dumps({"positions": []}))
245+
246+
assert should_simulate(str(dataset_path)) is False
247+
assert (dataset_path / "point_dataset.json").exists()
248+
249+
250+
def test__full_regime__unreadable_data_fits__is_kept(monkeypatch, tmp_path):
251+
# "Unknown regime" must never mean "delete".
252+
monkeypatch.delenv("PYAUTO_SMALL_DATASETS", raising=False)
253+
dataset_path = tmp_path / "dataset"
254+
dataset_path.mkdir()
255+
(dataset_path / "data.fits").write_bytes(b"not a fits file")
256+
257+
assert _on_disk_shape_native(dataset_path / "data.fits") is None
258+
assert should_simulate(str(dataset_path)) is False
259+
assert (dataset_path / "data.fits").exists()
260+
261+
262+
def test__is_small_datasets_on_disk__reads_shape_from_header(tmp_path):
263+
small = _write_dataset(tmp_path / "small", SMALL_DATASETS_SHAPE_NATIVE)
264+
full = _write_dataset(tmp_path / "full", (180, 180))
265+
266+
assert _is_small_datasets_on_disk(str(small)) is True
267+
assert _is_small_datasets_on_disk(str(full)) is False
268+
269+
270+
def test__on_disk_shape_native__is_row_column_ordered(tmp_path):
271+
# NAXIS1 is columns and NAXIS2 is rows, so a non-square array must come
272+
# back in numpy (rows, columns) order rather than transposed.
273+
dataset_path = _write_dataset(tmp_path / "dataset", (30, 50))
274+
275+
assert _on_disk_shape_native(dataset_path / "data.fits") == (30, 50)

0 commit comments

Comments
 (0)