From e693eefa033c5d7a7e95c2787ac3f4dbbce0d2c3 Mon Sep 17 00:00:00 2001 From: Peter Corke Date: Fri, 31 Jul 2026 17:36:18 +1000 Subject: [PATCH 1/2] docs: log Histogram.plot type= docstring/behavior mismatch Found while adding type annotations to plot() -- the docstring lists 'frequency'/'cdf'/'ncdf' as the accepted type= values, but the actual dispatch also accepts 'pdf'/'probability'/'cf'/'cumulative'/'normalized' and does not handle 'ncdf' at all. Left type annotated as plain str rather than Literal for this reason -- reconciling docstring vs behavior is a separate decision, out of scope for an annotations-only pass. --- tech-debt.md | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/tech-debt.md b/tech-debt.md index 93975529..a385b3fe 100644 --- a/tech-debt.md +++ b/tech-debt.md @@ -1,5 +1,26 @@ # Technical Debt +## `Histogram.plot`'s `type` docstring doesn't match its actual accepted values + +Found 2026-07-30 while adding type annotations to `Histogram.plot` +(`ImageWholeFeatures.py:1200`). The docstring says `type` accepts +`'frequency'` [default], `'cdf'`, or `'ncdf'`. The actual dispatch logic +in the method body accepts a different, larger set: +`'frequency'`, `'pdf'`/`'probability'`, `'cf'`/`'cumulative'`, +`'cdf'`/`'normalized'` — and does **not** handle `'ncdf'` at all (it +would fall through to the `else: raise ValueError("unknown type")` +branch). Left `type` annotated as plain `str` rather than a `Literal` +enum for this reason — using `Literal` would mean either copying the +stale docstring's wrong values or silently fixing behavior/docs as a +drive-by, both out of scope for an annotations-only pass. + +### Fix + +Reconcile the docstring with the real accepted values (or vice versa, +if `'ncdf'` was meant to work and was dropped by accident — check git +blame). Once settled, `type` can become +`Literal["frequency", "pdf", "probability", "cf", "cumulative", "cdf", "normalized"]`. + ## [HIGH PRIORITY] mypy is not run anywhere in CI or dev tooling Found 2026-07-29 while fixing the `_ImageBase` Protocol gaps in From 7e8cabe4ceee1f517b1a9161ab28e9d87137475e Mon Sep 17 00:00:00 2001 From: Peter Corke Date: Fri, 31 Jul 2026 17:44:55 +1000 Subject: [PATCH 2/2] fix: add missing type annotations, warn on ncdf deprecation Type annotations (finding 6): - Histogram.plot(): full signature was completely unannotated. Added types to every parameter and the return type. Deliberately kept the type= parameter name as-is despite shadowing the builtin -- it's a public keyword argument (hist.plot(type="pdf")), renaming it is a breaking change out of scope here. Left it typed as plain str rather than Literal (see tech-debt.md entry added alongside this commit -- the docstring's listed values don't match what the method actually accepts). - ImageRegionFeatures.py: __str__/__repr__ (x2 each) -> str - Sources.py: _open_reader() -> Any (conditionally-imported ROS reader type behind an optional dependency; Any matches the existing pattern used elsewhere in this file for the same reason) Deprecation warnings (finding 7, corrected from the original review -- the ImageCore.py "A" property version-drift claim didn't reproduce, both sides already said 2.0.0): - ImageWholeFeaturesMixin.ncdf (Image.ncdf) and Histogram.ncdf both had a .. deprecated:: 2.0.0 docstring note but neither ever called warnings.warn, unlike every other deprecated method in the codebase. Added the standard warning to both, with messages matching what each actually delegates to (hist().cdf for the Image-level property, plain .cdf for the Histogram-level one -- also fixed the second property's docstring, which incorrectly said "hist().cdf" despite self already being the Histogram instance). Added a regression test for the new Histogram.ncdf warning and updated the existing (already-passing) Image.ncdf test to also assert the warning. Verified genuinely: reverted just the two warnings.warn calls (via git stash on the source file only, keeping the tests), confirmed both fail with "DeprecationWarning not triggered", restored the fix, confirmed both pass. Full suite: 817 passed (816 + 1 net new test), 15 skipped, no regressions. --- .../ImageRegionFeatures.py | 8 ++-- .../ImageWholeFeatures.py | 41 ++++++++++++------- src/machinevisiontoolbox/Sources.py | 2 +- tests/test_image_whole_features.py | 14 ++++++- 4 files changed, 43 insertions(+), 22 deletions(-) diff --git a/src/machinevisiontoolbox/ImageRegionFeatures.py b/src/machinevisiontoolbox/ImageRegionFeatures.py index 9d160f54..78abfc5d 100644 --- a/src/machinevisiontoolbox/ImageRegionFeatures.py +++ b/src/machinevisiontoolbox/ImageRegionFeatures.py @@ -242,7 +242,7 @@ def __getitem__(self, i: int | slice | np.ndarray | list | tuple) -> "MSERFeatur return new - def __str__(self): + def __str__(self) -> str: """ String representation of MSER @@ -265,7 +265,7 @@ def __str__(self): s = f"MSER feature: u: {self._bboxes[0,0]} - {self._bboxes[0,2]}, v: {self._bboxes[0,1]} - {self._bboxes[0,3]}" return s - def __repr__(self): + def __repr__(self) -> str: """ Representation of MSER @@ -373,7 +373,7 @@ def __init__(self, ocr: dict, i: int) -> None: for key in ocr.keys(): self.dict[key] = ocr[key][i] - def __str__(self): + def __str__(self) -> str: """ String representation of MSER @@ -382,7 +382,7 @@ def __str__(self): """ return f"{self.dict['text']} ({self.dict['conf']}%)" - def __repr__(self): + def __repr__(self) -> str: return str(self) @property diff --git a/src/machinevisiontoolbox/ImageWholeFeatures.py b/src/machinevisiontoolbox/ImageWholeFeatures.py index 3b20aeed..eeae1a71 100644 --- a/src/machinevisiontoolbox/ImageWholeFeatures.py +++ b/src/machinevisiontoolbox/ImageWholeFeatures.py @@ -11,6 +11,7 @@ import matplotlib.pyplot as plt import numpy as np import scipy as sp +from matplotlib.axes import Axes from matplotlib.patches import Polygon from matplotlib.ticker import ScalarFormatter from spatialmath import SE3, base @@ -519,6 +520,11 @@ def ncdf(self) -> np.ndarray: .. deprecated:: 2.0.0 Use ``hist().cdf`` instead. """ + warnings.warn( + "Deprecated in 2.0.0: use hist().cdf instead of ncdf.", + DeprecationWarning, + stacklevel=2, + ) hist = self._default_hist() return hist.cdf @@ -1193,26 +1199,31 @@ def ncdf(self) -> np.ndarray: :rtype: ndarray(N) or ndarray(N,P) .. deprecated:: 2.0.0 - Use ``hist().cdf`` instead. + Use ``cdf`` instead. """ + warnings.warn( + "Deprecated in 2.0.0: use .cdf instead of .ncdf.", + DeprecationWarning, + stacklevel=2, + ) return self.cdf def plot( self, - type="frequency", - block=False, - filled=None, - stats=True, - style="stack", - cursor=False, - alpha=0.5, - title=None, - log=False, - samescale=False, - ax=None, - bar=None, - **kwargs, - ): + type: str = "frequency", + block: bool = False, + filled: bool | None = None, + stats: bool = True, + style: str = "stack", + cursor: bool = False, + alpha: float = 0.5, + title: str | None = None, + log: bool = False, + samescale: bool = False, + ax: Axes | None = None, + bar: bool | None = None, + **kwargs: Any, + ) -> None: """ Plot histogram diff --git a/src/machinevisiontoolbox/Sources.py b/src/machinevisiontoolbox/Sources.py index c4336282..3658fd95 100644 --- a/src/machinevisiontoolbox/Sources.py +++ b/src/machinevisiontoolbox/Sources.py @@ -3113,7 +3113,7 @@ def _close_reader(self) -> None: reader.close() self.reader = None - def _open_reader(self): + def _open_reader(self) -> Any: if self.reader is not None: return self.reader diff --git a/tests/test_image_whole_features.py b/tests/test_image_whole_features.py index a5986f3a..4c88c23b 100644 --- a/tests/test_image_whole_features.py +++ b/tests/test_image_whole_features.py @@ -463,13 +463,23 @@ def test_cdf_property(self): # new test def test_ncdf_property(self): - """Test normalized CDF property""" + """Test normalized CDF property, and that it warns as deprecated""" im = Image.Random(size=(50, 50), dtype="uint8") - ncdf = im.ncdf + with self.assertWarns(DeprecationWarning): + ncdf = im.ncdf self.assertIsNotNone(ncdf) # Normalized CDF should end at 1.0 # self.assertAlmostEqual(ncdf[-1], 1.0) + def test_histogram_ncdf_deprecated(self): + """Histogram.ncdf (as opposed to Image.ncdf above) should also warn, + and should still return the same values as Histogram.cdf""" + im = Image.Random(size=(50, 50), dtype="uint8") + h = im.hist() + with self.assertWarns(DeprecationWarning): + ncdf = h.ncdf + nt.assert_array_equal(ncdf, h.cdf) + if __name__ == "__main__": unittest.main()