Skip to content

Commit c0c00b0

Browse files
Jammy2211claude
authored andcommitted
fix: widen scalar pixel_scales / shape_native at the two sites #464 missed
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Fj1HoQa4hZPmbyyBYNJX62
1 parent 0f75c3d commit c0c00b0

5 files changed

Lines changed: 150 additions & 12 deletions

File tree

autoarray/geometry/geometry_util.py

Lines changed: 14 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -7,25 +7,34 @@
77

88
def convert_shape_native_1d(shape_native: Union[int, Tuple[int]]) -> Tuple[int]:
99
"""
10-
Convert an input `shape_native` of type `int` to a tuple `(int,)`. If the input is already a
11-
`(int,)` tuple it is returned unchanged.
10+
Convert an input `shape_native` given as a single integer scalar to a tuple `(int,)`. If
11+
the input is already a `(int,)` tuple it is returned unchanged.
1212
1313
This enables users to input `shape_native` as a single integer value and have the type
1414
automatically normalised to `(int,)` which is used internally by 1D data structures.
1515
16+
Any concrete integer scalar is widened — `int` and `np.integer` — not just an exact `int`.
17+
An `np.integer` is what indexing a shape tuple or reading a FITS header returns, so it
18+
reaches this function on paths a user would consider ordinary. The widened value is cast
19+
to a Python `int`, so the tuple this returns is always `(int,)` regardless of what went in.
20+
21+
A `float` is deliberately *not* widened: `shape_native` counts pixels, so a non-integer is
22+
a mistake worth surfacing rather than one to normalise away. Neither is a `bool`, nor a JAX
23+
tracer — see :func:`autoarray.validate.is_concrete_integer`.
24+
1625
Parameters
1726
----------
1827
shape_native
19-
The 1D shape to convert, either as a plain `int` or a 1-element tuple `(int,)`.
28+
The 1D shape to convert, either as a plain integer scalar or a 1-element tuple `(int,)`.
2029
2130
Returns
2231
-------
2332
Tuple[int]
2433
The shape as a 1-element tuple `(int,)`.
2534
"""
2635

27-
if type(shape_native) is int:
28-
shape_native = (shape_native,)
36+
if validate.is_concrete_integer(shape_native):
37+
shape_native = (int(shape_native),)
2938

3039
return shape_native
3140

autoarray/mask/mask_1d.py

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010

1111
from autoarray.mask.derive.grid_1d import DeriveGrid1D
1212
from autoarray.mask.derive.mask_1d import DeriveMask1D
13+
from autoarray.geometry import geometry_util
1314
from autoarray.geometry.geometry_1d import Geometry1D
1415
from autoarray.structures.abstract_structure import Structure
1516
from autoarray.structures.arrays import array_1d_util
@@ -68,8 +69,7 @@ def __init__(
6869
if invert:
6970
mask = ~mask
7071

71-
if type(pixel_scales) is float:
72-
pixel_scales = (pixel_scales,)
72+
pixel_scales = geometry_util.convert_pixel_scales_1d(pixel_scales=pixel_scales)
7373

7474
if len(mask.shape) != 1:
7575
raise exc.MaskException("The input mask is not a one dimensional array")

autoarray/validate.py

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,29 @@ def is_concrete_scalar(value: Any) -> bool:
7070
return isinstance(value, (int, float, np.integer, np.floating))
7171

7272

73+
def is_concrete_integer(value: Any) -> bool:
74+
"""
75+
Returns ``True`` if ``value`` is a concrete Python or NumPy **integer** scalar.
76+
77+
The integer-only counterpart of :func:`is_concrete_scalar`, for parameters which
78+
count pixels rather than measure them — a ``shape_native`` of ``5.0`` is a mistake
79+
worth surfacing, not a value to silently widen, so a ``float`` returns ``False``
80+
here where ``is_concrete_scalar`` would accept it.
81+
82+
Tracer-safety and the ``bool`` exclusion carry over from
83+
:func:`is_concrete_scalar` unchanged.
84+
85+
Parameters
86+
----------
87+
value
88+
The value to test.
89+
"""
90+
if isinstance(value, bool):
91+
return False
92+
93+
return isinstance(value, (int, np.integer))
94+
95+
7396
def _raise(
7497
name: str,
7598
rule: str,

test_autoarray/geometry/test_geometry_util.py

Lines changed: 70 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -21,18 +21,25 @@ def test__convert_pixel_scales_1d__widens_any_real_scalar(pixel_scales):
2121
"""
2222
Any concrete real scalar widens, not just an exact `float`. `int` is what a user types by
2323
hand; `np.floating` is what indexing an array or reading a FITS header returns.
24+
25+
The tuple-ness is asserted before the value: `np.float64(1.0) == (1.0,)` NumPy-broadcasts
26+
to `array([True])`, which is truthy, so a value-only assertion passes on the unwidened
27+
scalar and tests nothing.
2428
"""
25-
assert aa.util.geometry.convert_pixel_scales_1d(pixel_scales=pixel_scales) == (1.0,)
29+
pixel_scales = aa.util.geometry.convert_pixel_scales_1d(pixel_scales=pixel_scales)
30+
31+
assert type(pixel_scales) is tuple
32+
assert pixel_scales == (1.0,)
2633

2734

2835
@pytest.mark.parametrize(
2936
"pixel_scales", [1, 1.0, np.float64(1.0), np.int32(1), np.float32(1.0)]
3037
)
3138
def test__convert_pixel_scales_2d__widens_any_real_scalar(pixel_scales):
32-
assert aa.util.geometry.convert_pixel_scales_2d(pixel_scales=pixel_scales) == (
33-
1.0,
34-
1.0,
35-
)
39+
pixel_scales = aa.util.geometry.convert_pixel_scales_2d(pixel_scales=pixel_scales)
40+
41+
assert type(pixel_scales) is tuple
42+
assert pixel_scales == (1.0, 1.0)
3643

3744

3845
@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():
8693
assert aa.util.geometry.convert_pixel_scales_2d(pixel_scales=True) is True
8794

8895

96+
@pytest.mark.parametrize("shape_native", [5, np.int32(5), np.int64(5)])
97+
def test__convert_shape_native_1d__widens_any_integer_scalar(shape_native):
98+
"""
99+
Any concrete integer scalar widens, not just an exact `int`. An `np.integer` is what
100+
indexing a shape tuple or reading a FITS header returns, and `Array1D.full` then does
101+
`shape_native[0]` on whatever comes back — a bare scalar raised `IndexError` there.
102+
103+
The tuple-ness is asserted before the value: `np.int32(5) == (5,)` NumPy-broadcasts to
104+
`array([True])`, which is truthy, so a value-only assertion passes on the unwidened
105+
scalar and tests nothing.
106+
"""
107+
shape_native = aa.util.geometry.convert_shape_native_1d(shape_native=shape_native)
108+
109+
assert type(shape_native) is tuple
110+
assert shape_native == (5,)
111+
112+
113+
@pytest.mark.parametrize("shape_native", [5, np.int32(5), np.int64(5)])
114+
def test__convert_shape_native_1d__widened_entry_is_a_python_int(shape_native):
115+
"""
116+
The widened value is cast, so a NumPy scalar never reaches the shape stored on a
117+
structure. `5 == np.int32(5)` in Python, so the cast has to be asserted on the type.
118+
"""
119+
(entry,) = aa.util.geometry.convert_shape_native_1d(shape_native=shape_native)
120+
assert type(entry) is int
121+
122+
123+
def test__convert_shape_native_1d__a_float_is_not_widened():
124+
"""
125+
Unlike `pixel_scales`, `shape_native` counts pixels rather than measuring them, so a
126+
`float` is a mistake worth surfacing rather than one to normalise away — the predicate
127+
here is `is_concrete_integer`, not `is_concrete_scalar`.
128+
"""
129+
assert aa.util.geometry.convert_shape_native_1d(shape_native=5.0) == 5.0
130+
131+
132+
def test__convert_shape_native_1d__a_bool_is_not_treated_as_a_scalar():
133+
"""`bool` is a subclass of `int`, but `True` reaching a pixel count is a different mistake."""
134+
assert aa.util.geometry.convert_shape_native_1d(shape_native=True) is True
135+
136+
137+
def test__convert_shape_native_1d__tuple_input_is_returned_unchanged():
138+
shape_native = (5,)
139+
assert (
140+
aa.util.geometry.convert_shape_native_1d(shape_native=shape_native)
141+
is shape_native
142+
)
143+
144+
145+
def test__convert_shape_native_1d__a_tracer_passes_through_untouched():
146+
"""Inside a `jax.jit` the value is traced; widening it would resolve it to a bool."""
147+
tracer_like = _NotAConcreteScalar()
148+
149+
assert (
150+
aa.util.geometry.convert_shape_native_1d(shape_native=tracer_like) is tracer_like
151+
)
152+
153+
89154
def test__central_pixel_coordinates_1d_from():
90155
central_pixel_coordinates = aa.util.geometry.central_pixel_coordinates_1d_from(
91156
shape_slim=(3,)

test_autoarray/mask/test_mask_1d.py

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,47 @@ def test__constructor__input_is_2d_mask__raises_exception():
5555
aa.Mask1D(mask=[[False, False, True]], pixel_scales=1.0)
5656

5757

58+
@pytest.mark.parametrize(
59+
"pixel_scales", [1, 1.0, np.float64(1.0), np.int32(1), np.float32(1.0)]
60+
)
61+
def test__constructor__widens_any_real_scalar_pixel_scales(pixel_scales):
62+
"""
63+
`Mask1D` hand-rolled its own `type(pixel_scales) is float` check and never routed through
64+
`convert_pixel_scales_1d`, so an `int` or a NumPy scalar was stored bare. `Mask2D` already
65+
went through the chokepoint — this closed the 1D/2D divergence.
66+
"""
67+
mask = aa.Mask1D(mask=[False, False, True], pixel_scales=pixel_scales)
68+
69+
assert mask.pixel_scales == (1.0,)
70+
assert type(mask.pixel_scales[0]) is float
71+
72+
73+
def test__constructor__scalar_pixel_scales__geometry_is_usable():
74+
"""
75+
The bare scalar only surfaced later, when geometry subscripted it:
76+
`TypeError: 'int' object is not subscriptable`, naming nothing the caller passed.
77+
"""
78+
mask = aa.Mask1D(mask=[False, False, True], pixel_scales=1)
79+
80+
assert mask.geometry.scaled_maxima == (1.5,)
81+
82+
83+
def test__constructor__tuple_pixel_scales_returned_unchanged():
84+
mask = aa.Mask1D(mask=[False, False, True], pixel_scales=(1.0,))
85+
86+
assert mask.pixel_scales == (1.0,)
87+
88+
89+
@pytest.mark.parametrize("pixel_scales", [0, 0.0, -1, -1.0, float("nan")])
90+
def test__constructor__invalid_pixel_scales__raises_exception(pixel_scales):
91+
"""
92+
Routing through `convert_pixel_scales_1d` brings `validate.validate_pixel_scales` with it,
93+
so `Mask1D` now rejects what `Mask2D` already rejected. A deliberate contract change.
94+
"""
95+
with pytest.raises(ValueError):
96+
aa.Mask1D(mask=[False, False, True], pixel_scales=pixel_scales)
97+
98+
5899
# ---------------------------------------------------------------------------
59100
# is_all_true / is_all_false — parametrized
60101
# ---------------------------------------------------------------------------

0 commit comments

Comments
 (0)