From 67bd84fe50692848542aad4fdd159c9b8362390b Mon Sep 17 00:00:00 2001 From: Peter Corke Date: Sun, 2 Aug 2026 08:45:58 +1000 Subject: [PATCH 1/6] refactor: extract _infer_dtype/_reshape_to_size from Image.__init__ Image.__init__ was 379 lines doing several unrelated jobs. Extracted two self-contained pieces: - _infer_dtype(image, dtype): staticmethod, pure function -- resolves the constructor's dtype= argument (auto-detect smallest int type, inherit via dtype=True, validate an explicit dtype, or reject dtype=False). Zero risk: no self access at all. - _reshape_to_size(self, image, size): the size= constructor argument handling -- reshaping 1D/oddly-oriented 2D input into the right (height, width, planes) shape. Added comprehensive test coverage first (TestImageInferDtype, TestImageReshapeToSize in test_image_core.py -- 22 new tests, one per branch/combination), since this code had real coverage gaps: no test for dtype=False, no test for either of the two "color plane count mismatch" ValueError paths in the reshape logic. All 22 pass against the extracted code. That coverage made a follow-up safe: _reshape_to_size had a `self.colororder is not None` branch that's provably dead code -- self._colororder is set to None at the very start of __init__ and never touched before this point, so the branch can never run. Confirmed via the getter (`return self._colororder`, no other logic) and by grepping for any other write to self._colororder earlier in the method -- none. Removed it and the now-unused color_dict parameter it was the only consumer of. Verified: full suite 840 passed (818 baseline + 22 new tests), 15 skipped, no regressions -- including immediately after the dead-code removal, confirming it was a true no-op as expected. --- src/machinevisiontoolbox/ImageCore.py | 109 +++++++++++----------- tests/test_image_core.py | 124 ++++++++++++++++++++++++++ 2 files changed, 182 insertions(+), 51 deletions(-) diff --git a/src/machinevisiontoolbox/ImageCore.py b/src/machinevisiontoolbox/ImageCore.py index 2a581a14..13b810fa 100644 --- a/src/machinevisiontoolbox/ImageCore.py +++ b/src/machinevisiontoolbox/ImageCore.py @@ -301,6 +301,57 @@ def __init__( ) # if dtype is not given, determine the appropriate type for the data + dtype = self._infer_dtype(image, dtype) + + if binary: + image = image > 0 + + # change type of array to the determined dtype + if dtype is not None: + image = image.astype(dtype, copy=False) + + self.name = name + + color_dict = Image.colororder2dict(colororder) + + image = self._reshape_to_size(image, size) + + if image.ndim not in (2, 3): + raise ValueError( + "bad ndarray passed to Image constructor: must be 2D or 3D array" + ) + if image.ndim == 3 and image.shape[2] == 1: + image = image[:, :, 0] # squeeze out singleton plane + + if kwargs: + image = convert(image, **kwargs) + + # assign the image to the object, copying if requested + if copy: + self._A = image.copy() + else: + self._A = image + + # final check that colororder length matches number of planes + if colororder is not None: + if len(color_dict) != self.nplanes: + raise ValueError("colororder length does not match number of planes") + + if colororder is None: + if self.nplanes == 3: + self.colororder = "RGB" + # warnings.warn("defaulting color to RGB") + else: + self.colororder = color_dict + + self._stats = None + + self.name = name + + @staticmethod + def _infer_dtype(image: np.ndarray, dtype: Dtype | bool | None) -> np.dtype | None: + """Determine the pixel dtype for the constructor when ``dtype`` isn't + given explicitly, or resolve/validate it when it is.""" if dtype is None: # no type given, automatically choose it if np.issubdtype(image.dtype, np.floating): @@ -335,17 +386,12 @@ def __init__( except TypeError: raise ValueError("bad dtype argument passed to Image constructor") - if binary: - image = image > 0 - - # change type of array to the determined dtype - if dtype is not None: - image = image.astype(dtype, copy=False) - - self.name = name - - color_dict = Image.colororder2dict(colororder) + return dtype + def _reshape_to_size( + self, image: np.ndarray, size: tuple | list | None + ) -> np.ndarray: + """Reshape 1D/2D pixel data per the constructor's ``size=`` argument.""" if isinstance(size, self.__class__): # size is an Image instance, ignore the size/shape and use the image's shape size = size.size @@ -354,16 +400,7 @@ def __init__( if size is not None: newsize = [size[1], size[0]] - if self.colororder is not None: - nplanes = len(color_dict) - - if len(size) == 3: - if nplanes != size[2]: - raise ValueError( - "colororder length does not match number of planes in size" - ) - newsize.append(nplanes) - elif len(size) == 3: + if len(size) == 3: newsize.append(size[2]) if image.ndim == 1: @@ -420,37 +457,7 @@ def __init__( image = image.reshape(*newsize) - if image.ndim not in (2, 3): - raise ValueError( - "bad ndarray passed to Image constructor: must be 2D or 3D array" - ) - if image.ndim == 3 and image.shape[2] == 1: - image = image[:, :, 0] # squeeze out singleton plane - - if kwargs: - image = convert(image, **kwargs) - - # assign the image to the object, copying if requested - if copy: - self._A = image.copy() - else: - self._A = image - - # final check that colororder length matches number of planes - if colororder is not None: - if len(color_dict) != self.nplanes: - raise ValueError("colororder length does not match number of planes") - - if colororder is None: - if self.nplanes == 3: - self.colororder = "RGB" - # warnings.warn("defaulting color to RGB") - else: - self.colororder = color_dict - - self._stats = None - - self.name = name + return image @staticmethod def _plane_stats(plane: np.ndarray) -> dict[str, float | int]: diff --git a/tests/test_image_core.py b/tests/test_image_core.py index 94ff4588..0f82ba2f 100644 --- a/tests/test_image_core.py +++ b/tests/test_image_core.py @@ -1238,6 +1238,130 @@ def test_init_singleton_removal(self): self.assertEqual(im.nplanes, 1) +class TestImageInferDtype(unittest.TestCase): + """Image._infer_dtype(), extracted from __init__. One test per branch.""" + + def test_no_dtype_float_input(self): + im = Image(np.array([[1.0, 2.0], [3.0, 4.0]])) + self.assertEqual(im.dtype, np.dtype(np.float32)) + + def test_no_dtype_signed_int_fits_int8(self): + im = Image(np.array([[-1, 2], [3, 4]])) + self.assertEqual(im.dtype, np.dtype("int8")) + + def test_no_dtype_signed_int_fits_int16_not_int8(self): + im = Image(np.array([[-1, 200], [3, 4]])) + self.assertEqual(im.dtype, np.dtype("int16")) + + def test_no_dtype_signed_int_fits_int32_not_int16(self): + im = Image(np.array([[-1, 40000], [3, 4]])) + self.assertEqual(im.dtype, np.dtype("int32")) + + def test_no_dtype_unsigned_int_fits_uint8(self): + im = Image(np.array([[1, 2], [3, 4]])) + self.assertEqual(im.dtype, np.dtype("uint8")) + + def test_no_dtype_unsigned_int_fits_uint16_not_uint8(self): + im = Image(np.array([[1, 300], [3, 4]])) + self.assertEqual(im.dtype, np.dtype("uint16")) + + def test_no_dtype_unsigned_int_fits_uint32_not_uint16(self): + im = Image(np.array([[1, 70000], [3, 4]])) + self.assertEqual(im.dtype, np.dtype("uint32")) + + def test_dtype_true_inherits_input_dtype(self): + im = Image(np.ones((2, 3), dtype="int16"), dtype=True) + self.assertEqual(im.dtype, np.dtype("int16")) + + def test_dtype_false_raises(self): + with self.assertRaises(ValueError): + Image(np.ones((2, 3)), dtype=False) + + def test_dtype_explicit_string(self): + im = Image(np.ones((2, 3)), dtype="uint8") + self.assertEqual(im.dtype, np.dtype("uint8")) + + def test_dtype_invalid_raises(self): + with self.assertRaises(ValueError): + Image(np.ones((2, 3)), dtype="not-a-real-dtype") + + +class TestImageReshapeToSize(unittest.TestCase): + """Image._reshape_to_size(), extracted from __init__. One test per + combination of (size type) x (image.ndim) x (wide/tall) x (2- vs + 3-element newsize), including the mismatch error paths.""" + + def test_size_from_image_instance(self): + template = Image.Zeros(size=(5, 6)) + im = Image(np.zeros(30), size=template) + self.assertEqual(im.size, (5, 6)) + + def test_1d_array_2tuple_size(self): + im = Image(np.arange(12.0), size=(4, 3)) + self.assertEqual(im.shape, (3, 4)) + + def test_1d_array_3tuple_size_multiplane(self): + x = np.arange(2 * 3 * 3, dtype="uint8") + im = Image(x, size=(3, 2, 3), colororder="RGB") + self.assertEqual(im.shape, (2, 3, 3)) + + def test_2d_wide_2tuple_size_single_row_not_appended(self): + # shape (1, 12): wide (shape[1] > shape[0]), 2-element newsize, + # shape[0] == 1 so it is NOT appended as a 3rd dimension + im = Image(np.arange(12.0).reshape(1, -1), size=(4, 3)) + self.assertEqual(im.shape, (3, 4)) + self.assertEqual(im.nplanes, 1) + + def test_2d_wide_2tuple_size_multirow_appended(self): + # shape (3, 6): wide, 2-element newsize, shape[0] == 3 > 1 so it + # IS appended -> 3 planes + x = np.arange(18.0).reshape(3, 6) + im = Image(x, size=(2, 3), colororder="RGB") + self.assertEqual(im.shape, (3, 2, 3)) + self.assertEqual(im.nplanes, 3) + + def test_2d_wide_3tuple_size_matching_planes(self): + # shape (3, 6): wide, explicit size=(w, h, 3) matches shape[0] == 3 + x = np.arange(18.0).reshape(3, 6) + im = Image(x, size=(2, 3, 3), colororder="RGB") + self.assertEqual(im.shape, (3, 2, 3)) + + def test_2d_wide_3tuple_size_mismatched_planes_raises(self): + # shape (3, 6): wide, explicit size=(w, h, 4) does NOT match + # shape[0] == 3 + x = np.arange(18.0).reshape(3, 6) + with self.assertRaises(ValueError): + Image(x, size=(2, 3, 4), colororder="RGBA") + + def test_2d_tall_2tuple_size_single_column_not_appended(self): + # shape (12, 1): tall (shape[1] <= shape[0]), 2-element newsize, + # shape[1] == 1 so it is NOT appended + im = Image(np.arange(12.0).reshape(-1, 1), size=(4, 3)) + self.assertEqual(im.shape, (3, 4)) + self.assertEqual(im.nplanes, 1) + + def test_2d_tall_2tuple_size_multicolumn_appended(self): + # shape (6, 3): tall, 2-element newsize, shape[1] == 3 > 1 so it + # IS appended -> 3 planes + x = np.arange(18.0).reshape(6, 3) + im = Image(x, size=(3, 2), colororder="RGB") + self.assertEqual(im.shape, (2, 3, 3)) + self.assertEqual(im.nplanes, 3) + + def test_2d_tall_3tuple_size_matching_planes(self): + # shape (6, 3): tall, explicit size=(w, h, 3) matches shape[1] == 3 + x = np.arange(18.0).reshape(6, 3) + im = Image(x, size=(3, 2, 3), colororder="RGB") + self.assertEqual(im.shape, (2, 3, 3)) + + def test_2d_tall_3tuple_size_mismatched_planes_raises(self): + # shape (6, 3): tall, explicit size=(w, h, 4) does NOT match + # shape[1] == 3 + x = np.arange(18.0).reshape(6, 3) + with self.assertRaises(ValueError): + Image(x, size=(3, 2, 4), colororder="RGBA") + + # ------------------------------------------------------------------------ # if __name__ == "__main__": unittest.main() From d56ab592f428b821172fe282ee2d3712f241f43d Mon Sep 17 00:00:00 2001 From: Peter Corke Date: Sun, 2 Aug 2026 09:55:37 +1000 Subject: [PATCH 2/6] refactor: extract _fixdims/_lenkey from Image.__getitem__ The nested fixdims()/lenkey() closures captured zero outer variables -- pure functions of their own parameters -- the lowest-risk extraction of this whole hygiene pass. Pulled out as _lenkey (staticmethod) and _fixdims (classmethod, calls cls._lenkey), matching the _infer_dtype/_plane_stats staticmethod pattern already used elsewhere in this class. Minor bonus: no longer recreates a closure on every __getitem__ call. Also closed real coverage gaps found while verifying this: no test exercised a stepped slice (im[::2, ::3], the n // step arithmetic branch in _lenkey, as opposed to the span-1 shortcut), and neither of the two "invalid slice" ValueError paths (wrong number of slice elements, or a key type that's neither int/str/tuple/list) had any test at all. Full suite: 843 passed (840 + 3 new tests), 15 skipped, no regressions. --- src/machinevisiontoolbox/ImageCore.py | 93 ++++++++++++++------------- tests/test_image_core.py | 22 +++++++ 2 files changed, 69 insertions(+), 46 deletions(-) diff --git a/src/machinevisiontoolbox/ImageCore.py b/src/machinevisiontoolbox/ImageCore.py index 13b810fa..7658c89a 100644 --- a/src/machinevisiontoolbox/ImageCore.py +++ b/src/machinevisiontoolbox/ImageCore.py @@ -2718,50 +2718,6 @@ def __getitem__( :seealso: :meth:`red` :meth:`green` :meth:`blue` :meth:`plane` :meth:`roi` :meth:`pixel` """ - def fixdims( - out: np.ndarray, shape: tuple[int, ...], keys: tuple[Any, ...] - ) -> np.ndarray: - # deal with the fact that some of the keys may have reduced the - # dimensionality of the array, eg. a slice of span 0 or 1, or an integer - # key. - - # shape: (nrows, ncols, nplanes) in NumPy order - # key: (rowspec, colspec, planespec) in NumPy order - - def lenkey(key: int | slice, max: int) -> int: - # compute the span of a particular key - if isinstance(key, int): - # int key has a span on 1 - return 1 - elif isinstance(key, slice): - # slice key has a span depending on the slice and the corresponding - # array dimension. we have to essentially replicate the logic of - # slice() here. - start = key.start if key.start is not None else 0 - stop = key.stop if key.stop is not None else max - step = key.step if key.step is not None else 1 - n = stop - start - if n < step: - return 1 - else: - return n // step - - dims = list(shape) - - if len(shape) == 2: - dims = [lenkey(keys[0], shape[0]), lenkey(keys[1], shape[1])] - elif len(shape) == 3: - dims = [ - lenkey(keys[0], shape[0]), - lenkey(keys[1], shape[1]), - lenkey(keys[2], shape[2]), - ] - # ignore loss of color dimension - if dims[2] == 1: - dims = dims[:2] - - return out.reshape(tuple(dims)) - if ( isinstance(keys, tuple) and len(keys) == 2 @@ -2798,7 +2754,7 @@ def lenkey(key: int | slice, max: int) -> int: out = self._A[keys] if out.ndim < 3: - out = fixdims(out, self._A.shape, keys) + out = self._fixdims(out, self._A.shape, keys) else: # greyscale image @@ -2811,7 +2767,7 @@ def lenkey(key: int | slice, max: int) -> int: out = self._A[keys] if out.ndim < 2: - out = fixdims(out, self._A.shape, keys) + out = self._fixdims(out, self._A.shape, keys) # a singleton plane dimensions is a grey scale image if out.ndim == 3 and out.shape[2] == 1: @@ -2832,6 +2788,51 @@ def lenkey(key: int | slice, max: int) -> int: else: raise ValueError("invalid slice") + @staticmethod + def _lenkey(key: int | slice, max: int) -> int: + """Span of a single ``__getitem__`` key component (int or slice).""" + if isinstance(key, int): + # int key has a span on 1 + return 1 + elif isinstance(key, slice): + # slice key has a span depending on the slice and the corresponding + # array dimension. we have to essentially replicate the logic of + # slice() here. + start = key.start if key.start is not None else 0 + stop = key.stop if key.stop is not None else max + step = key.step if key.step is not None else 1 + n = stop - start + if n < step: + return 1 + else: + return n // step + + @classmethod + def _fixdims( + cls, out: np.ndarray, shape: tuple[int, ...], keys: tuple[Any, ...] + ) -> np.ndarray: + """Restore dimensionality lost by ``__getitem__`` keys that collapse + an axis (eg. a slice of span 0 or 1, or an integer key). + + ``shape``: (nrows, ncols, nplanes) in NumPy order. + ``keys``: (rowspec, colspec, planespec) in NumPy order. + """ + dims = list(shape) + + if len(shape) == 2: + dims = [cls._lenkey(keys[0], shape[0]), cls._lenkey(keys[1], shape[1])] + elif len(shape) == 3: + dims = [ + cls._lenkey(keys[0], shape[0]), + cls._lenkey(keys[1], shape[1]), + cls._lenkey(keys[2], shape[2]), + ] + # ignore loss of color dimension + if dims[2] == 1: + dims = dims[:2] + + return out.reshape(tuple(dims)) + def pixel(self, u: int, v: int) -> int | float | np.ndarray: """ Return pixel value diff --git a/tests/test_image_core.py b/tests/test_image_core.py index 0f82ba2f..880628f4 100644 --- a/tests/test_image_core.py +++ b/tests/test_image_core.py @@ -121,6 +121,28 @@ def test_getitem_grey(self): sim = im[5:6, 6:7] self.assertEqual(sim.size, (1, 1)) + def test_getitem_stepped_slice(self): + # exercises _lenkey's step != 1 arithmetic (n // step), not just + # the span-1 (int key / zero-span slice) shortcut + im = Image(np.arange(80).reshape((10, 8)), dtype="int64") # 8x10 image + sim = im[0:8:2, 0:10:3] + self.assertEqual(sim.size, (4, 4)) + nt.assert_array_equal(sim.array, im.array[0:10:3, 0:8:2]) + + def test_getitem_invalid_number_of_slices(self): + im = Image(np.arange(80).reshape((10, 8)), dtype="int64") + with self.assertRaises(ValueError): + im[0:4, 0:4, 0:1, 0:1] + + im_color = Image(np.arange(240).reshape((10, 8, 3)), dtype="int64") + with self.assertRaises(ValueError): + im_color[0:4, 0:4, 0:1, 0:1] + + def test_getitem_invalid_key_type(self): + im = Image(np.arange(80).reshape((10, 8)), dtype="int64") + with self.assertRaises(ValueError): + im[1.5] + def test_colordict(self): cdict = Image.colororder2dict("RGBA") self.assertIsInstance(cdict, dict) From 5278bd5e5563f15050ad986db2bc6edfe6d454d1 Mon Sep 17 00:00:00 2001 From: Peter Corke Date: Sun, 2 Aug 2026 10:43:20 +1000 Subject: [PATCH 3/6] test: add characterization tests for Camera.plot() CameraBase.plot() had zero test coverage before this -- adding it first, ahead of an upcoming extract-method refactor, so there's an actual before/after baseline instead of "trust me it still works". Asserts on artist counts (ax.collections/ax.lines) for shape="frustum", shape="camera", and frame=True, not deep rendering correctness. Found and fixed a real test-isolation bug while getting these to pass in the full suite: plot()'s ax=None path reuses whatever matplotlib considers the "current" 3D axes (spatialmath.base.graphics.axes_logic docs: "checks for a match with the passed axes ax or the current axes"). Without closing figures in setUp, a figure left open by an unrelated, earlier test in the suite got silently reused, making the artist-count assertions meaningless (40 collections instead of 3, passed in isolation, failed in the full run). Added plt.close("all") to both setUp and tearDown. Full suite: 846 passed (843 + 3 new tests), 15 skipped, no regressions, stable across repeated runs. --- tests/test_camera.py | 43 +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 43 insertions(+) diff --git a/tests/test_camera.py b/tests/test_camera.py index 7a3f1bfb..649d1053 100644 --- a/tests/test_camera.py +++ b/tests/test_camera.py @@ -352,6 +352,49 @@ def test_camera_clone_methods(self): pass +class TestCameraPlot(unittest.TestCase): + """Characterization tests for CameraBase.plot(). This method had zero + test coverage before an extract-method refactor -- these establish a + concrete before/after baseline (artist counts on the returned axes) + rather than deeply validating rendering correctness.""" + + def _camera(self): + return CentralCamera( + f=0.015, rho=10e-6, imagesize=[1280, 1024], pp=[640, 512], name="cam1" + ) + + def setUp(self): + # plot()'s ax=None path reuses whatever matplotlib considers the + # "current" 3D axes (spatialmath.base.graphics.axes_logic) -- close + # everything first so a figure left open by an unrelated test + # elsewhere in the suite can't be silently reused here, which would + # make the artist counts below meaningless. + plt.close("all") + + def tearDown(self): + plt.close("all") + + def test_plot_frustum(self): + ax = self._camera().plot(shape="frustum") + self.assertIsNotNone(ax) + self.assertEqual(len(ax.collections), 1) + self.assertEqual(len(ax.lines), 0) + + def test_plot_camera(self): + ax = self._camera().plot(shape="camera") + self.assertIsNotNone(ax) + self.assertEqual(len(ax.collections), 3) + self.assertEqual(len(ax.lines), 0) + + def test_plot_frame(self): + # shape defaults to "camera", so this also draws the camera icon + # in addition to the pose-frame overlay + ax = self._camera().plot(frame=True) + self.assertIsNotNone(ax) + self.assertEqual(len(ax.collections), 4) + self.assertEqual(len(ax.lines), 3) + + # ----------------------------------------------------------------------- # if __name__ == "__main__": From 7aed3095e5035955f73e4b7a38d46c2117107112 Mon Sep 17 00:00:00 2001 From: Peter Corke Date: Sun, 2 Aug 2026 13:44:43 +1000 Subject: [PATCH 4/6] refactor: extract _plot_frustum/_plot_camera_icon from Camera.plot CameraBase.plot() mixed a matplotlib-version-compat shim, two independent icon-drawing paths (frustum vs camera+cylinder), and a pose-frame overlay in one 175-line method. Extracted the two self-contained icon-drawing paths as _plot_frustum and _plot_camera_icon; left the compat shim (try/finally around ax.add_collection3d) and the frame overlay in plot() itself, since those aren't independent sub-tasks. Preserved verbatim rather than "cleaned up": an unused local variable (`a = 3 # length of axis line segments` in the camera-icon branch, never referenced) -- out of scope for a behavior-preserving extraction. Verified behavior-preserving: the characterization tests added in the previous commit (which had zero coverage before this refactor started) show byte-identical artist counts before and after this extraction. Full suite: 846 passed, 15 skipped, no regressions. --- src/machinevisiontoolbox/Camera.py | 171 ++++++++++++++++------------- 1 file changed, 93 insertions(+), 78 deletions(-) diff --git a/src/machinevisiontoolbox/Camera.py b/src/machinevisiontoolbox/Camera.py index 55ae67e9..791ae69d 100644 --- a/src/machinevisiontoolbox/Camera.py +++ b/src/machinevisiontoolbox/Camera.py @@ -1216,86 +1216,10 @@ def _add_collection3d_compat(collection, *args, **kwargs): try: # draw camera-like object: if shape == "frustum": - # TODO make this kwargs or optional inputs - # side colors: - # +x red - # -y red - # +y green - # -y yellow - length = scale - widthb = scale / 10 - widtht = scale - widthb /= 2 - widtht /= 2 - b0 = np.array([-widthb, -widthb, 0, 1]) - b1 = np.array([-widthb, widthb, 0, 1]) - b2 = np.array([widthb, widthb, 0, 1]) - b3 = np.array([widthb, -widthb, 0, 1]) - t0 = np.array([-widtht, -widtht, length, 1]) - t1 = np.array([-widtht, widtht, length, 1]) - t2 = np.array([widtht, widtht, length, 1]) - t3 = np.array([widtht, -widtht, length, 1]) - - # bottom/narrow end - T = pose.A - b0 = (T @ b0)[:-1] - b1 = (T @ b1)[:-1] - b2 = (T @ b2)[:-1] - b3 = (T @ b3)[:-1] - - # wide/top end - t0 = (T @ t0)[:-1] - t1 = (T @ t1)[:-1] - t2 = (T @ t2)[:-1] - t3 = (T @ t3)[:-1] - - points = [ - np.array([b0, b1, t1, t0]), # -x face - np.array([b1, b2, t2, t1]), # +y face - np.array([b2, b3, t3, t2]), # +x face - np.array([b3, b0, t0, t3]), # -y face - ] - poly = Poly3DCollection( - points, facecolors=["r", "g", "r", "y"], alpha=alpha - ) - ax.add_collection3d(poly) + self._plot_frustum(ax, pose, scale, alpha) elif shape == "camera": - # the box is centred at the origin and its centerline parallel to the - # z-axis. Its z-extent is -bh/2 to bh/2. - W = 0.5 # width & height of the box - L = 1.2 # length of the box - cr = 0.2 # cylinder radius - ch = 0.4 # cylinder height - cn = 12 # number of facets of cylinder - a = 3 # length of axis line segments - - # draw the box part of the camera - smb.plot_cuboid( - sides=np.r_[W, W, L] * scale, - pose=pose, - filled=solid, - color=color, - alpha=0.5 * alpha if solid else alpha, - ax=ax, - ) - - # draw the lens - smb.plot_cylinder( - radius=cr * scale, - height=np.r_[L / 2, L / 2 + ch] * scale, - resolution=cn, - pose=pose, - filled=solid, - color=color, - alpha=0.5 * alpha, - ax=ax, - ) - - if label: - ax.set_xlabel("X") - ax.set_ylabel("Y") - ax.set_zlabel("Z") + self._plot_camera_icon(ax, pose, scale, solid, color, alpha, label) if frame is True: self.pose.plot( @@ -1311,6 +1235,97 @@ def _add_collection3d_compat(collection, *args, **kwargs): return ax + def _plot_frustum(self, ax: Axes, pose: SE3, scale: float, alpha: float) -> None: + """Draw the ``shape="frustum"`` camera icon into ``ax``.""" + # TODO make this kwargs or optional inputs + # side colors: + # +x red + # -y red + # +y green + # -y yellow + length = scale + widthb = scale / 10 + widtht = scale + widthb /= 2 + widtht /= 2 + b0 = np.array([-widthb, -widthb, 0, 1]) + b1 = np.array([-widthb, widthb, 0, 1]) + b2 = np.array([widthb, widthb, 0, 1]) + b3 = np.array([widthb, -widthb, 0, 1]) + t0 = np.array([-widtht, -widtht, length, 1]) + t1 = np.array([-widtht, widtht, length, 1]) + t2 = np.array([widtht, widtht, length, 1]) + t3 = np.array([widtht, -widtht, length, 1]) + + # bottom/narrow end + T = pose.A + b0 = (T @ b0)[:-1] + b1 = (T @ b1)[:-1] + b2 = (T @ b2)[:-1] + b3 = (T @ b3)[:-1] + + # wide/top end + t0 = (T @ t0)[:-1] + t1 = (T @ t1)[:-1] + t2 = (T @ t2)[:-1] + t3 = (T @ t3)[:-1] + + points = [ + np.array([b0, b1, t1, t0]), # -x face + np.array([b1, b2, t2, t1]), # +y face + np.array([b2, b3, t3, t2]), # +x face + np.array([b3, b0, t0, t3]), # -y face + ] + poly = Poly3DCollection(points, facecolors=["r", "g", "r", "y"], alpha=alpha) + ax.add_collection3d(poly) + + def _plot_camera_icon( + self, + ax: Axes, + pose: SE3, + scale: float, + solid: bool, + color: str, + alpha: float, + label: bool, + ) -> None: + """Draw the ``shape="camera"`` box+cylinder icon into ``ax``.""" + # the box is centred at the origin and its centerline parallel to the + # z-axis. Its z-extent is -bh/2 to bh/2. + W = 0.5 # width & height of the box + L = 1.2 # length of the box + cr = 0.2 # cylinder radius + ch = 0.4 # cylinder height + cn = 12 # number of facets of cylinder + a = 3 # length of axis line segments + + # draw the box part of the camera + smb.plot_cuboid( + sides=np.r_[W, W, L] * scale, + pose=pose, + filled=solid, + color=color, + alpha=0.5 * alpha if solid else alpha, + ax=ax, + ) + + # draw the lens + smb.plot_cylinder( + radius=cr * scale, + height=np.r_[L / 2, L / 2 + ch] * scale, + resolution=cn, + pose=pose, + filled=solid, + color=color, + alpha=0.5 * alpha, + ax=ax, + ) + + if label: + ax.set_xlabel("X") + ax.set_ylabel("Y") + ax.set_zlabel("Z") + def _add_noise_distortion(self, uv: np.ndarray) -> np.ndarray: """ Add noise to pixel coordinates From 06353ddddbda2ae8178e5a25722e805f278b1a43 Mon Sep 17 00:00:00 2001 From: Peter Corke Date: Sun, 2 Aug 2026 14:00:43 +1000 Subject: [PATCH 5/6] test: add Histogram.plot(style="overlay") happy-path coverage The only existing overlay-style test (test_hist_sorted_overlay_raises) covers the error path only -- nothing exercised actual overlay rendering (color assignment, filled polygons) before this. Adding ahead of an upcoming extract-method refactor of plot(), same reasoning as the Camera.plot() characterization tests: real coverage before touching the code, not after. Full suite: 847 passed (846 + 1 new test), 15 skipped, no regressions. --- tests/test_image_whole_features.py | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) diff --git a/tests/test_image_whole_features.py b/tests/test_image_whole_features.py index a5986f3a..e99e218a 100644 --- a/tests/test_image_whole_features.py +++ b/tests/test_image_whole_features.py @@ -110,6 +110,23 @@ def test_hist_sorted_overlay_raises(self): with self.assertRaises(ValueError): h.plot(style="overlay", block=False) + def test_hist_overlay_happy_path(self): + """style="overlay" rendering, as opposed to the error-path test + above -- there was previously no test that ever reached the + overlay-rendering code at all.""" + im = Image(np.random.randint(0, 255, (20, 20, 3), dtype=np.uint8)) + hist = im.hist() + + with patch("matplotlib.pyplot.show"): + hist.plot(style="overlay", filled=True, block=False) + + fig = plt.gcf() + self.assertEqual(len(fig.axes), 1) + ax = fig.axes[0] + self.assertEqual(len(ax.patches), 3) # one filled polygon per plane + self.assertIsNotNone(hist.colordict) + plt.close(fig) + def test_hist_opt_sorted_deprecated(self): im = Image.String("000011112222") with self.assertWarns(DeprecationWarning): From 822e242904c459c6f63690b2930eeef0d90b1cce Mon Sep 17 00:00:00 2001 From: Peter Corke Date: Sun, 2 Aug 2026 14:06:45 +1000 Subject: [PATCH 6/6] refactor: extract 3 helpers from Histogram.plot Histogram.plot() was ~420 lines. Extracted the three low-closure- capture pieces: - _compute_plot_series(type, samescale): the type-dispatch block that computes y/maxy/ylabel1/ylabel2. Pure computation over self.h/ self.pdf/self.cf/self.cdf. The `if filled is None: filled = True` side effect that lived inside the "frequency" branch stays in plot() itself as a one-line follow-up, since it mutates a variable used well beyond this block. - _histogram_plane_stats(counts): was a nested plane_stats() closure, called from both the stack and overlay branches (real duplication removed, not just relocated). - _resolve_overlay_colors(n, colors): per-plane color assignment for style="overlay". Mutates self.colordict as a side effect when unset -- preserved exactly, documented in the new method's docstring rather than silently dropped. Deliberately NOT extracted (per the plan for this hygiene pass): the stack-loop body, the cursor sub-block (nested on_move() closure capturing ~8 outer locals), or splitting stack/overlay into top-level methods -- all have heavy closure capture where extraction would just relocate the long-parameter-list smell into explicit arguments. Verified behavior-preserving: the 5 plot-related tests (including the two added in the previous two commits specifically to cover this method before touching it) pass with byte-identical assertions before and after. Full suite: 847 passed, 15 skipped, no regressions, stable across repeated runs. --- .../ImageWholeFeatures.py | 144 +++++++++++------- 1 file changed, 87 insertions(+), 57 deletions(-) diff --git a/src/machinevisiontoolbox/ImageWholeFeatures.py b/src/machinevisiontoolbox/ImageWholeFeatures.py index 3b20aeed..8d69582b 100644 --- a/src/machinevisiontoolbox/ImageWholeFeatures.py +++ b/src/machinevisiontoolbox/ImageWholeFeatures.py @@ -1343,38 +1343,9 @@ def plot( samescale = True # figure vertical axis labels and scaling based on histogram type - if type == "frequency": - y = self.h - if samescale: - maxy = np.max(y) - else: - maxy = np.max(y, axis=0) - ylabel1 = "frequency" - ylabel2 = "frequency" - if filled is None: - filled = True - elif type in ("pdf", "probability"): - y = self.pdf - if samescale: - maxy = np.max(y) - else: - maxy = np.max(y, axis=0) - ylabel1 = "PDF" - ylabel2 = "probability density" - elif type in ("cf", "cumulative"): - y = self.cf - maxy = y[ - -1, 0 - ] # last row values are all the same, total number of pixels in plane - ylabel1 = "cumulative frequency" - ylabel2 = "cumulative frequency" - elif type in ("cdf", "normalized"): - y = self.cdf - maxy = 1 - ylabel1 = "CDF" - ylabel2 = "normalized cumulative frequency" - else: - raise ValueError("unknown type") + y, maxy, ylabel1, ylabel2 = self._compute_plot_series(type, samescale) + if type == "frequency" and filled is None: + filled = True if self.nplanes == 1: y = y[..., np.newaxis] @@ -1391,17 +1362,6 @@ def plot( if self.nplanes == 1: hist_counts = hist_counts[..., np.newaxis] - def plane_stats(counts: np.ndarray) -> tuple[float | None, float | None]: - total = float(np.sum(counts)) - if total <= 0: - return None, None - - mean = float(np.sum(self.x * counts) / total) - cdf = np.cumsum(counts) - median_idx = int(np.searchsorted(cdf, 0.5 * total, side="left")) - median = float(self.x[min(median_idx, len(self.x) - 1)]) - return mean, median - if self._sorted: xrange = (self.x[0], self.x[-1]) xlabel = "bin rank" @@ -1448,7 +1408,7 @@ def plane_stats(counts: np.ndarray) -> tuple[float | None, float | None]: ScalarFormatter(useOffset=False, useMathText=True) ) if stats: - mean, median = plane_stats(hist_counts[:, i]) + mean, median = self._histogram_plane_stats(hist_counts[:, i]) if mean is not None and median is not None: ax.axvline( mean, @@ -1549,18 +1509,7 @@ def on_move(event): x = np.r_[xrange[0], x, xrange[1]] _, ax = plt.subplots(1, 1) - patchcolor = [] - goodcolors = [c for c in "rgbykcm"] - if self.colordict is None: - self.colordict = {c: i for i, c in enumerate(goodcolors[:n])} - colors = list(self.colordict.keys()) - patchcolor = [c.lower() for c in colors] - else: - for color, i in self.colordict.items(): - if color.lower() in "rgbykcm": - patchcolor.append(color.lower()) - else: - patchcolor.append(goodcolors.pop(0)) + colors, patchcolor = self._resolve_overlay_colors(n, colors) if filled: for i in range(n): @@ -1580,7 +1529,7 @@ def on_move(event): ax.plot(x, np.r_[0, y[:, i], 0], color=patchcolor[i], **kwargs) if stats: - mean, median = plane_stats(np.sum(hist_counts, axis=1)) + mean, median = self._histogram_plane_stats(np.sum(hist_counts, axis=1)) if mean is not None and median is not None: ax.axvline( mean, @@ -1617,6 +1566,87 @@ def on_move(event): set_window_title(title) safe_plt_show(block=block) + def _compute_plot_series( + self, type: str, samescale: bool + ) -> tuple[np.ndarray, np.ndarray | float, str, str]: + """Compute plot()'s y-values, y-axis max, and y-axis labels for a + given ``type``. Pure computation over self.h/self.pdf/self.cf/ + self.cdf -- no mutation.""" + if type == "frequency": + y = self.h + if samescale: + maxy = np.max(y) + else: + maxy = np.max(y, axis=0) + ylabel1 = "frequency" + ylabel2 = "frequency" + elif type in ("pdf", "probability"): + y = self.pdf + if samescale: + maxy = np.max(y) + else: + maxy = np.max(y, axis=0) + ylabel1 = "PDF" + ylabel2 = "probability density" + elif type in ("cf", "cumulative"): + y = self.cf + maxy = y[ + -1, 0 + ] # last row values are all the same, total number of pixels in plane + ylabel1 = "cumulative frequency" + ylabel2 = "cumulative frequency" + elif type in ("cdf", "normalized"): + y = self.cdf + maxy = 1 + ylabel1 = "CDF" + ylabel2 = "normalized cumulative frequency" + else: + raise ValueError("unknown type") + + return y, maxy, ylabel1, ylabel2 + + def _histogram_plane_stats( + self, counts: np.ndarray + ) -> tuple[float | None, float | None]: + """Mean and median of a single plane's histogram counts, or + (None, None) if the plane is empty. Used by both plot()'s stack + and overlay styles.""" + total = float(np.sum(counts)) + if total <= 0: + return None, None + + mean = float(np.sum(self.x * counts) / total) + cdf = np.cumsum(counts) + median_idx = int(np.searchsorted(cdf, 0.5 * total, side="left")) + median = float(self.x[min(median_idx, len(self.x) - 1)]) + return mean, median + + def _resolve_overlay_colors( + self, n: int, colors: list[str] + ) -> tuple[list[str], list[str]]: + """Resolve per-plane display colors for plot()'s style="overlay". + + If self.colordict is unset, assigns a default one as a side effect + (preserved intentionally -- this is existing behavior, not + introduced by extracting this into its own method) and returns the + colors derived from it; otherwise maps the existing colordict's + color names onto a fallback palette for any non-standard names. + """ + patchcolor = [] + goodcolors = [c for c in "rgbykcm"] + if self.colordict is None: + self.colordict = {c: i for i, c in enumerate(goodcolors[:n])} + colors = list(self.colordict.keys()) + patchcolor = [c.lower() for c in colors] + else: + for color, i in self.colordict.items(): + if color.lower() in "rgbykcm": + patchcolor.append(color.lower()) + else: + patchcolor.append(goodcolors.pop(0)) + + return colors, patchcolor + def peaks(self, **kwargs: Any) -> np.ndarray | list[np.ndarray]: r""" Histogram peaks