From c0c00b0e1a66afc8b300e41bb088ed7fe4e9a648 Mon Sep 17 00:00:00 2001 From: Jammy2211 Date: Sun, 23 Aug 2026 17:47:54 -0400 Subject: [PATCH] fix: widen scalar pixel_scales / shape_native at the two sites #464 missed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PyAutoArray#464 (`8298d74e`) replaced `type(x) is float` with `validate.is_concrete_scalar` in `convert_pixel_scales_1d` and `convert_pixel_scales_2d`, so any concrete real scalar widens to the tuple form both functions promise. Re-running that prompt's repro found the sweep did not reach every site of the same defect. Two were still live on main. `Mask1D.__init__` hand-rolled its own widening and never routed through `convert_pixel_scales_1d`, so it still carried the original exact-type check. `Mask1D(mask=..., pixel_scales=1)` stored the bare `1`, and the mask's geometry then raised `TypeError: 'int' object is not subscriptable` — #464's exact reported symptom, on a public constructor. `Mask2D.__init__` already called `convert_pixel_scales_2d`, and `Grid1D.uniform` reaches the chokepoint too, so this was a 1D/2D divergence rather than a design choice. It now makes the same call `Mask2D` makes. That also brings `validate.validate_pixel_scales` to `Mask1D`, which is a deliberate contract change: `Mask1D` now rejects `0`, negative and `nan` pixel scales exactly as `Mask2D` already did. No test constructed one that way and all 12 library call sites pass real scales, so nothing needed adjusting to suit it. `convert_shape_native_1d` kept `type(shape_native) is int`, which `8298d74e` listed as not-fixed-there. `Array1D.full` is its sole caller and does `shape_native[0]` on the result, so `Array1D.full(shape_native=np.int32(5))` raised `IndexError: invalid index to scalar variable`. It now tests `validate.is_concrete_integer` and casts to a Python `int`. `is_concrete_integer` is new, beside `is_concrete_scalar`: `shape_native` counts pixels rather than measuring them, so `is_concrete_scalar` is the wrong predicate there — it would silently widen a `float`, which is a mistake worth surfacing. `bool` exclusion and tracer-safety carry over unchanged, so both functions stay safe inside a `jax.jit`; verified by compiling and running one. Also tightened #464's own widening tests. `np.float64(1.0) == (1.0,)` NumPy-broadcasts to `array([True])`, which is truthy, so their value-only assertions passed on an unwidened NumPy scalar and tested nothing. Asserting tuple-ness before the value makes them fail on the pre-#464 source (confirmed by reverting it), where four of the six parametrisations previously passed vacuously. The new tests here assert the same way for the same reason. Not fixed here, needing its own change: tuple entries are still returned unnormalised, so `convert_pixel_scales_2d((1, 1))` keeps its ints and contradicts the `Tuple[float, float]` annotation. That alters return values on paths which work today. Validation: 1201 passed / 0 failed on the full test_autoarray suite. The 3 pre-existing pynufft failures `8298d74e` baselined no longer occur, so there was nothing to baseline against. Every new assertion that claims regression coverage was confirmed to fail without the source change; the boundary tests (tuple unchanged, float/bool not widened, tracer passthrough) pass either way by design, mirroring the ones #464 shipped. Downstream blast radius is nil: PyAutoGalaxy and PyAutoLens only re-export `Mask1D` and construct none, and neither uses `Array1D.full`/`zeros`/`ones`. Closes #484 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Fj1HoQa4hZPmbyyBYNJX62 --- autoarray/geometry/geometry_util.py | 19 +++-- autoarray/mask/mask_1d.py | 4 +- autoarray/validate.py | 23 ++++++ test_autoarray/geometry/test_geometry_util.py | 75 +++++++++++++++++-- test_autoarray/mask/test_mask_1d.py | 41 ++++++++++ 5 files changed, 150 insertions(+), 12 deletions(-) diff --git a/autoarray/geometry/geometry_util.py b/autoarray/geometry/geometry_util.py index d3e83cc37..5f63cbf6d 100644 --- a/autoarray/geometry/geometry_util.py +++ b/autoarray/geometry/geometry_util.py @@ -7,16 +7,25 @@ def convert_shape_native_1d(shape_native: Union[int, Tuple[int]]) -> Tuple[int]: """ - Convert an input `shape_native` of type `int` to a tuple `(int,)`. If the input is already a - `(int,)` tuple it is returned unchanged. + Convert an input `shape_native` given as a single integer scalar to a tuple `(int,)`. If + the input is already a `(int,)` tuple it is returned unchanged. This enables users to input `shape_native` as a single integer value and have the type automatically normalised to `(int,)` which is used internally by 1D data structures. + Any concrete integer scalar is widened — `int` and `np.integer` — not just an exact `int`. + An `np.integer` is what indexing a shape tuple or reading a FITS header returns, so it + reaches this function on paths a user would consider ordinary. The widened value is cast + to a Python `int`, so the tuple this returns is always `(int,)` regardless of what went in. + + A `float` is deliberately *not* widened: `shape_native` counts pixels, so a non-integer is + a mistake worth surfacing rather than one to normalise away. Neither is a `bool`, nor a JAX + tracer — see :func:`autoarray.validate.is_concrete_integer`. + Parameters ---------- shape_native - The 1D shape to convert, either as a plain `int` or a 1-element tuple `(int,)`. + The 1D shape to convert, either as a plain integer scalar or a 1-element tuple `(int,)`. Returns ------- @@ -24,8 +33,8 @@ def convert_shape_native_1d(shape_native: Union[int, Tuple[int]]) -> Tuple[int]: The shape as a 1-element tuple `(int,)`. """ - if type(shape_native) is int: - shape_native = (shape_native,) + if validate.is_concrete_integer(shape_native): + shape_native = (int(shape_native),) return shape_native diff --git a/autoarray/mask/mask_1d.py b/autoarray/mask/mask_1d.py index 24e3e226e..511266e2a 100644 --- a/autoarray/mask/mask_1d.py +++ b/autoarray/mask/mask_1d.py @@ -10,6 +10,7 @@ from autoarray.mask.derive.grid_1d import DeriveGrid1D from autoarray.mask.derive.mask_1d import DeriveMask1D +from autoarray.geometry import geometry_util from autoarray.geometry.geometry_1d import Geometry1D from autoarray.structures.abstract_structure import Structure from autoarray.structures.arrays import array_1d_util @@ -68,8 +69,7 @@ def __init__( if invert: mask = ~mask - if type(pixel_scales) is float: - pixel_scales = (pixel_scales,) + pixel_scales = geometry_util.convert_pixel_scales_1d(pixel_scales=pixel_scales) if len(mask.shape) != 1: raise exc.MaskException("The input mask is not a one dimensional array") diff --git a/autoarray/validate.py b/autoarray/validate.py index b7e40802a..f95c9e979 100644 --- a/autoarray/validate.py +++ b/autoarray/validate.py @@ -70,6 +70,29 @@ def is_concrete_scalar(value: Any) -> bool: return isinstance(value, (int, float, np.integer, np.floating)) +def is_concrete_integer(value: Any) -> bool: + """ + Returns ``True`` if ``value`` is a concrete Python or NumPy **integer** scalar. + + The integer-only counterpart of :func:`is_concrete_scalar`, for parameters which + count pixels rather than measure them — a ``shape_native`` of ``5.0`` is a mistake + worth surfacing, not a value to silently widen, so a ``float`` returns ``False`` + here where ``is_concrete_scalar`` would accept it. + + Tracer-safety and the ``bool`` exclusion carry over from + :func:`is_concrete_scalar` unchanged. + + Parameters + ---------- + value + The value to test. + """ + if isinstance(value, bool): + return False + + return isinstance(value, (int, np.integer)) + + def _raise( name: str, rule: str, diff --git a/test_autoarray/geometry/test_geometry_util.py b/test_autoarray/geometry/test_geometry_util.py index a487bd005..bf5b64f2d 100644 --- a/test_autoarray/geometry/test_geometry_util.py +++ b/test_autoarray/geometry/test_geometry_util.py @@ -21,18 +21,25 @@ def test__convert_pixel_scales_1d__widens_any_real_scalar(pixel_scales): """ Any concrete real scalar widens, not just an exact `float`. `int` is what a user types by hand; `np.floating` is what indexing an array or reading a FITS header returns. + + The tuple-ness is asserted before the value: `np.float64(1.0) == (1.0,)` NumPy-broadcasts + to `array([True])`, which is truthy, so a value-only assertion passes on the unwidened + scalar and tests nothing. """ - assert aa.util.geometry.convert_pixel_scales_1d(pixel_scales=pixel_scales) == (1.0,) + pixel_scales = aa.util.geometry.convert_pixel_scales_1d(pixel_scales=pixel_scales) + + assert type(pixel_scales) is tuple + assert pixel_scales == (1.0,) @pytest.mark.parametrize( "pixel_scales", [1, 1.0, np.float64(1.0), np.int32(1), np.float32(1.0)] ) def test__convert_pixel_scales_2d__widens_any_real_scalar(pixel_scales): - assert aa.util.geometry.convert_pixel_scales_2d(pixel_scales=pixel_scales) == ( - 1.0, - 1.0, - ) + pixel_scales = aa.util.geometry.convert_pixel_scales_2d(pixel_scales=pixel_scales) + + assert type(pixel_scales) is tuple + assert pixel_scales == (1.0, 1.0) @pytest.mark.parametrize("pixel_scales", [1, np.float64(1.0), np.int32(1)]) @@ -86,6 +93,64 @@ def test__convert_pixel_scales__a_bool_is_not_treated_as_a_scalar(): assert aa.util.geometry.convert_pixel_scales_2d(pixel_scales=True) is True +@pytest.mark.parametrize("shape_native", [5, np.int32(5), np.int64(5)]) +def test__convert_shape_native_1d__widens_any_integer_scalar(shape_native): + """ + Any concrete integer scalar widens, not just an exact `int`. An `np.integer` is what + indexing a shape tuple or reading a FITS header returns, and `Array1D.full` then does + `shape_native[0]` on whatever comes back — a bare scalar raised `IndexError` there. + + The tuple-ness is asserted before the value: `np.int32(5) == (5,)` NumPy-broadcasts to + `array([True])`, which is truthy, so a value-only assertion passes on the unwidened + scalar and tests nothing. + """ + shape_native = aa.util.geometry.convert_shape_native_1d(shape_native=shape_native) + + assert type(shape_native) is tuple + assert shape_native == (5,) + + +@pytest.mark.parametrize("shape_native", [5, np.int32(5), np.int64(5)]) +def test__convert_shape_native_1d__widened_entry_is_a_python_int(shape_native): + """ + The widened value is cast, so a NumPy scalar never reaches the shape stored on a + structure. `5 == np.int32(5)` in Python, so the cast has to be asserted on the type. + """ + (entry,) = aa.util.geometry.convert_shape_native_1d(shape_native=shape_native) + assert type(entry) is int + + +def test__convert_shape_native_1d__a_float_is_not_widened(): + """ + Unlike `pixel_scales`, `shape_native` counts pixels rather than measuring them, so a + `float` is a mistake worth surfacing rather than one to normalise away — the predicate + here is `is_concrete_integer`, not `is_concrete_scalar`. + """ + assert aa.util.geometry.convert_shape_native_1d(shape_native=5.0) == 5.0 + + +def test__convert_shape_native_1d__a_bool_is_not_treated_as_a_scalar(): + """`bool` is a subclass of `int`, but `True` reaching a pixel count is a different mistake.""" + assert aa.util.geometry.convert_shape_native_1d(shape_native=True) is True + + +def test__convert_shape_native_1d__tuple_input_is_returned_unchanged(): + shape_native = (5,) + assert ( + aa.util.geometry.convert_shape_native_1d(shape_native=shape_native) + is shape_native + ) + + +def test__convert_shape_native_1d__a_tracer_passes_through_untouched(): + """Inside a `jax.jit` the value is traced; widening it would resolve it to a bool.""" + tracer_like = _NotAConcreteScalar() + + assert ( + aa.util.geometry.convert_shape_native_1d(shape_native=tracer_like) is tracer_like + ) + + def test__central_pixel_coordinates_1d_from(): central_pixel_coordinates = aa.util.geometry.central_pixel_coordinates_1d_from( shape_slim=(3,) diff --git a/test_autoarray/mask/test_mask_1d.py b/test_autoarray/mask/test_mask_1d.py index bf8a0a535..7f466d5b6 100644 --- a/test_autoarray/mask/test_mask_1d.py +++ b/test_autoarray/mask/test_mask_1d.py @@ -55,6 +55,47 @@ def test__constructor__input_is_2d_mask__raises_exception(): aa.Mask1D(mask=[[False, False, True]], pixel_scales=1.0) +@pytest.mark.parametrize( + "pixel_scales", [1, 1.0, np.float64(1.0), np.int32(1), np.float32(1.0)] +) +def test__constructor__widens_any_real_scalar_pixel_scales(pixel_scales): + """ + `Mask1D` hand-rolled its own `type(pixel_scales) is float` check and never routed through + `convert_pixel_scales_1d`, so an `int` or a NumPy scalar was stored bare. `Mask2D` already + went through the chokepoint — this closed the 1D/2D divergence. + """ + mask = aa.Mask1D(mask=[False, False, True], pixel_scales=pixel_scales) + + assert mask.pixel_scales == (1.0,) + assert type(mask.pixel_scales[0]) is float + + +def test__constructor__scalar_pixel_scales__geometry_is_usable(): + """ + The bare scalar only surfaced later, when geometry subscripted it: + `TypeError: 'int' object is not subscriptable`, naming nothing the caller passed. + """ + mask = aa.Mask1D(mask=[False, False, True], pixel_scales=1) + + assert mask.geometry.scaled_maxima == (1.5,) + + +def test__constructor__tuple_pixel_scales_returned_unchanged(): + mask = aa.Mask1D(mask=[False, False, True], pixel_scales=(1.0,)) + + assert mask.pixel_scales == (1.0,) + + +@pytest.mark.parametrize("pixel_scales", [0, 0.0, -1, -1.0, float("nan")]) +def test__constructor__invalid_pixel_scales__raises_exception(pixel_scales): + """ + Routing through `convert_pixel_scales_1d` brings `validate.validate_pixel_scales` with it, + so `Mask1D` now rejects what `Mask2D` already rejected. A deliberate contract change. + """ + with pytest.raises(ValueError): + aa.Mask1D(mask=[False, False, True], pixel_scales=pixel_scales) + + # --------------------------------------------------------------------------- # is_all_true / is_all_false — parametrized # ---------------------------------------------------------------------------