fix: prefer the FITS regime stamp over the shape heuristic in should_simulate - #474
Merged
Merged
Conversation
…simulate
Follows PyAutoNerves#153, which stamps SMALLDAT into every FITS the stack
writes. should_simulate now takes the regime from that card and keeps the shape
heuristic only as a fallback for datasets already on disk without one.
The three states are not interchangeable, and this predicate ends in
shutil.rmtree:
- SMALLDAT = T -> delete. The writer said so.
- SMALLDAT = F -> keep, unconditionally, without consulting shape. This also
retires a false positive: a dataset legitimately 16x16 at full resolution
was indistinguishable from a capped one by shape and was deleted on every
run.
- absent -> unknown, so fall back to shape. Never "full": every dataset
written before the stamp landed is absent.
A card that is not a genuine FITS boolean also reads as unknown. That is not
pedantry -- bool("F") is True in Python, so coercing a hand-edited or
third-party string card would invert the regime and hand a True to rmtree.
Interferometer datasets are now covered and were not before: their shape is
fixed by the committed uv file while the grid behind it is capped, so shape can
provably never see them. Tested directly.
The header key is duplicated here rather than imported from autonerves because
pyproject.toml floors autonerves at a release predating the stamp; an import
would hard-fail against a legitimately-resolved older version, whereas reading
an absent card degrades into exactly the fallback path above.
Scope stated honestly in the docstring: this reads <dataset_path>/data.fits and
covers ~228 of the 253 autolens_workspace call sites. Datacube (channel_XXX/),
multi_dataset ({waveband}_data.fits), sample (dataset_N/) and weak-lensing
(JSON-only) datasets have no file of that name at that level, get no verdict,
and fail safe to keep. Widening a destructive predicate's match is deliberately
left as its own change -- every trap recorded in autolens_workspace_test#260 was
a widened match hitting a file it should not have.
Corrects the issue text on one point: point-source datasets write a top-level
data.fits alongside their JSON and ARE covered. Only weak lensing is FITS-free.
Tests: the existing regression test's fixture wrote its "stale capped" dataset
with the cap env UNSET, so it now stamps F and describes a dataset that cannot
exist. Made faithful (written under the cap, read in the full regime) and split
three ways so the fallback stays exercised: stamped T deletes, unstamped 16x16
deletes via shape, stamped F at 16x16 is kept.
test_autoarray/structures/arrays/files/array/output_test/array.fits is a test WRITE TARGET, not a golden pin: the suite rewrites it on every run. Now that every FITS carries a SMALLDAT card its bytes differ from the committed copy, so leaving it stale would hand every contributor and CI run a dirty tree after testing. Verified against a true clean-main baseline (BOTH repos on main) that this dirtying is caused by the stamp and is not pre-existing. File size is unchanged at 5760 bytes -- a FITS header block holds 36 cards and this header carries far fewer, so the added card costs no bytes.
The two FITS-less dataset directories look like the worst gap and are actually the least urgent. dataset/weak/simple is regime-INVARIANT (nothing in its write path reads PYAUTO_SMALL_DATASETS, so capped and full runs produce an identical dataset.json and there is nothing to detect), and point_source/multiple_sources, which is genuinely regime-dependent, is excluded from harness execution by config/build/no_run.yaml pending PyAutoLens#480. Also drops the claim that the bug survives in every listed family -- it does not survive in weak/simple. Both supporting facts expire, so they are flagged as such and restated in the follow-up rather than left as silent assumptions.
…n it
The stamp and should_simulate were not about the same proposition.
stamp_small_datasets_regime records "the env var was set in the writing
process". should_simulate acted on it as "this data is capped, therefore stale
and disposable". The library itself already makes those diverge:
- Kernel2D.from_gaussian passes respect_small_datasets=False (convolver.py:729)
because a kernel's shape is intrinsic to the convolution operator, so a PSF
written under the cap is full resolution;
- Interferometer.from_fits applies no cap at all;
- and a user converting real telescope data in a shell exporting
PYAUTO_SMALL_DATASETS=1 -- the documented harness default -- stamps T on
genuinely full-resolution data.
Reproduced end to end: a 300x300 image written under the cap, then read in a
full-resolution run, was deleted. The pre-stamp shape heuristic explicitly
refused to delete that same file (_is_small_datasets_on_disk returned False), so
this was not a residual risk carried over -- it was a new deletion class, and a
strict weakening of the safety property PyAutoArray#471 established. The old
rule could not delete a 300x300 image under any circumstance; the new one could,
on the word of a header card that never inspected the data.
Every capped 2D image is rewritten to EXACTLY SMALL_DATASETS_SHAPE_NATIVE by
Grid2D.uniform or Mask2D.circular, so SMALLDAT=T on an image larger than the cap
in BOTH axes is a self-contradiction, and a predicate ending in shutil.rmtree
must resolve a contradiction toward keep.
Both axes, never either: interferometer data.fits is (n_visibilities, 2) --
108384 x 2 for the committed sdp81 dataset -- so an "either axis" test would
refuse to delete the exact family the stamp exists to catch. Verified lossless
against the real committed files: capped imaging (16,16) and interferometer
(108384,2) still delete; real imaging at 151x151, 209x209 and 300x300 is now
kept.
Unknown shape counts as not contradicted, since this guard only ever blocks a
deletion and must not silently protect a genuinely stale dataset.
Also fixes the interferometer test's array orientation to the real (n_vis, 2).
pyproject.toml floors autonerves at 2026.8.22.1, and that is currently the NEWEST release on PyPI -- so it predates the stamp. In any environment resolving autonerves from PyPI the writer emits no card, and tests that leaned on aa.output_to_fits to produce one would either fail outright or silently pass via the shape fallback while claiming to exercise the stamp. Force the stamp with _set_stamp instead. This suite owns the reader; the writer is tested in test_autonerves/test_fitsable.py. Verified green both against the stamped autonerves and against a simulated pre-stamp one.
Every FITS the stack writes now carries a SMALLDAT card whose value tracks PYAUTO_SMALL_DATASETS at write time. Several tests write into TRACKED fixture paths -- a pre-existing pattern, 14 such files across this repo and PyAutoNerves -- so the bytes those tests produce had become a function of the shell: running the suite with PYAUTO_SMALL_DATASETS=1 exported, which should_simulate's own docstring calls the default for most harness runs, passed but left the working tree dirty. Verified against fresh main worktrees that this dirtying is introduced by the stamp and is not pre-existing. An autouse fixture clearing the var restores the property the stamp took away -- test output is a function of the test, not of the environment -- in one place, rather than by rewriting every fixture-writing test in a PR about a header card. Tests that need a regime set it with monkeypatch.setenv in their body, which runs after the fixture and wins. No test depended on the ambient value. Verified: 1090 passed and tree clean both with the var exported and unset (the 11 failures are pre-existing missing-pynufft, identical on main). Found by three independent review lenses, each reproducing it separately.
6 tasks
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
Read side of PyAutoNerves#153. Merge PyAutoLabs/PyAutoNerves#154 first — see Merge ordering below.
should_simulatenow takes the resolution regime from theSMALLDATheader card that PyAutoNerves stamps into every FITS the stack writes, keeping the shape heuristic from #471 as the fallback for datasets already on disk without one.The three states are not interchangeable, and this predicate ends in
shutil.rmtree:SMALLDAT = TSMALLDAT = FThis closes the interferometer case, which shape provably cannot reach: its
data.fitsis(n_visibilities, 2)—108384 × 2for the committed sdp81 dataset — so a capped run writes identicalNAXISwith garbage values and trips no assertion at all.It also retires a false positive: a dataset legitimately 16×16 at full resolution was indistinguishable from a capped one by shape and was deleted on every run.
The safety guard, and why it exists
Adversarial review found — and reproduced — a data-loss regression in the first cut of this change, and it is the most important thing in this PR.
The stamp and this predicate were not about the same proposition. The stamp records "the env var was set in the writing process".
should_simulateacted on it as "this data is capped, therefore stale and disposable." The library already makes those diverge:Kernel2D.from_gaussianpassesrespect_small_datasets=False(convolver.py:729) so a PSF written under the cap is full resolution;Interferometer.from_fitsnever caps; and anyone converting real telescope data in a shell exportingPYAUTO_SMALL_DATASETS=1— the documented harness default — stampsTon genuinely full-resolution data.Reproduced end to end: a 300×300 image written under the cap, then read in a full run, was deleted — and the pre-stamp shape heuristic had explicitly refused to delete it. That made the first cut a strict weakening of the safety property #471 established, not a residual risk carried over.
Every capped 2D image is rewritten to exactly
SMALL_DATASETS_SHAPE_NATIVE, soSMALLDAT=Ton an image larger than the cap in both axes is a self-contradiction — and a predicate ending inrmtreeresolves a contradiction toward keep. Both axes, never either: interferometerdata.fitshasNAXIS1 = 2, so an "either axis" test would refuse to delete the one family this whole change exists for.Verified lossless against the real committed datasets:
data.fits(16,16)(108384,2)151×151/209×209/300×300API Changes
None — internal changes only.should_simulate's signature is unchanged and its contract is unchanged for every dataset already on disk (no stamp → prior shape behaviour). Behaviour changes only for stamped datasets, and every change is in the safe direction except the intended new detection of capped interferometer data.Test Plan
python -m pytest test_autoarray/— 1101 passed, 64 skipped, 0 failed(16,16)and(108384,2)still delete;151×151,209×209,300×300now keptbool("F")isTrue, so a hand-edited string card must never be coerced into a deletedata.fitsreads as unknown → keep*.fits" trapPYAUTO_SMALL_DATASETS=1exported and unsetMerge ordering
Merge PyAutoLabs/PyAutoNerves#154 first, and release it, before this.
pyproject.toml:30floorsautonerves>=2026.8.22.1, and that is currently the newest release on PyPI — i.e. the newest legitimately-resolvable autonerves predates the stamp. This is not a blocker: with an older autonerves the writer emits no card, readers see absent → unknown → the existing shape fallback, which is exactly the designed degradation. But until a stamped autonerves is released, this change has nothing to read and the interferometer case stays open.The header key is duplicated here rather than imported for that same reason — an import would hard-fail against a legitimately-resolved older autonerves, whereas an absent card degrades safely. It is a cross-repo wire-format contract with no compile-time link; any rename must be treated as such.
The tests deliberately force stamps with
_set_stamprather than relying onaa.output_to_fitsto write one, so this suite tests this repo's reader and passes under either autonerves version. The writer is tested intest_autonerves/test_fitsable.py.Known gap, stated rather than implied
This reads
<dataset_path>/data.fitsand nothing else — roughly 228 of the 253should_simulatecall sites in autolens_workspace. The rest get no verdict and fail safe to keep: datacube (channel_XXX/subdirs), multi_dataset ({waveband}_data.fits), sample (dataset_N/), and the two FITS-less directories.Widening the lookup is deliberately not done here. Every trap recorded in autolens_workspace_test#260 was a widened match hitting a file it should not have — a bare
*.fitsglob would delete every PSF-carrying dataset on every run. Growing a destructive predicate's reach is its own change with its own review.Two corrections to the original issue text, both verified: point-source datasets are not JSON-only — they write a top-level
data.fitsand are covered normally. Anddataset/weak/simple, the family the issue named, is regime-invariant (nothing in its write path reads the env var), so there is no bug there to fix. The one genuine JSON exposure,point_source/multiple_sources, is excluded from harness execution pending PyAutoLens#480 and is filed as a follow-up with both expiring justifications written down as explicit re-check triggers.Full API Changes (for automation & release notes)
Removed / Renamed / Changed Signature
None.
Added (private)
autoarray.util.dataset_util._small_datasets_stamp_on_disk(dataset_path)— tri-stateTrue/False/Noneread ofSMALLDATfromdata.fits. ReturnsNonefor missing, unreadable, absent-card and any card that is not a genuine FITS boolean.autoarray.util.dataset_util._stamp_contradicted_by_shape(dataset_path)—Truewhen the card claims capped but the shape says otherwise (larger than the cap in both axes). Unknown shape counts as not contradicted, since this guard only ever blocks a deletion.autoarray.util.dataset_util.SMALL_DATASETS_HEADER_KEY—"SMALLDAT", duplicated from autonerves by design (see Merge ordering).Changed Behaviour
should_simulate(dataset_path)— full-regime branch prefers the stamp, corroborates a destructiveTagainst the data, and falls back to the shape heuristic when unstamped. Small-regime branch unchanged.Migration
None. Unstamped datasets behave exactly as before this change.
Generated by the PyAutoLabs agent workflow.
Generated by Claude Code