Skip to content

fix: Image(dtype='float') resolved to float64, not float32 - #86

Open
petercorke wants to merge 1 commit into
mainfrom
fix/infer-dtype-alias-mismatch
Open

fix: Image(dtype='float') resolved to float64, not float32#86
petercorke wants to merge 1 commit into
mainfrom
fix/infer-dtype-alias-mismatch

Conversation

@petercorke

Copy link
Copy Markdown
Owner

Summary

Two independent, inconsistent dtype-string resolvers existed:

  • convert() had its own dtype_alias dict ('float'->'float32', 'double'->'float64', 'int'->'uint8', 'half'->'float16') since NumPy's own np.dtype('float') means float64, not float32.
  • Image.__init__'s _infer_dtype had no such table -- it called np.dtype(dtype) raw.

Since dtype is Image.__init__'s own named parameter (not forwarded via **kwargs to convert()), Image(arr, dtype='float', mono=True) sent 'float' through the unaliased path -> float64, while 'mono' went through convert()'s correctly-aliased one.

Root-caused via RVC3-python's chap11.ipynb motion-detection example: VideoFile("...", mono=True, dtype='float') produced float64 frames, and arithmetic on them stayed float64 all the way to Image.disp(matplotlib=False), where OpenCV's cvtColor doesn't support CV_64F -- 'Unsupported depth of input image'.

Fix: hoisted the alias table to a single shared constant, machinevisiontoolbox.base.types.DTYPE_ALIASES, used by both convert() and _infer_dtype. Documented the aliases via one Sphinx rst_epilog substitution (|dtype_aliases|, matching the existing |RVC3| pattern and bdsim's |BlockOptions| convention) referenced from both docstrings, rather than duplicating the explanation.

Since every ImageSource (VideoFile, VideoCamera, ImageCollection, ...) constructs Image instances with a dtype= option, this affects all of them uniformly, not just VideoFile.

Note on scope: this uncovered a broader pattern -- three independent, inconsistent dtype resolvers exist across this codebase. This PR fixes the constructor/convert() pair. Image.to()/.array_as()/.astype() (same alias bug) and the ImageConstantsMixin factory methods (Zeros/Constant/Random/... -- a different bug, they silently downcast even an explicit dtype='float64' request to float32 since they don't forward dtype= to the constructor) will be fixed on separate follow-up branches/PRs.

Test plan

  • New tests/test_dtype_resolution.py: a single parametrized matrix (every DTYPE_ALIASES case x every dtype-resolving entry point fixed so far) rather than scattered one-off tests -- designed to grow a test method per entry point as the follow-up PRs land, so this whole bug class stays pinned down in one place
  • Verified against pre-fix code: constructor cases fail with the exact original bug shapes (int64 instead of uint8, float64 instead of float32); pass with the fix
  • Full tests/test_image_core.py + tests/base/test_io.py + tests/test_dtype_resolution.py suites pass (220 passed)

Two independent, inconsistent dtype-string resolvers existed:
convert() had its own dtype_alias dict ('float'->'float32', 'double'
->'float64', 'int'->'uint8', 'half'->'float16') since NumPy's own
np.dtype('float') means float64, not float32. Image.__init__'s
_infer_dtype had no such table -- it called np.dtype(dtype) raw. Since
dtype is Image.__init__'s own named parameter (not forwarded via
**kwargs to convert()), Image(arr, dtype='float', mono=True) sent
'float' through the unaliased path -> float64, while 'mono' went
through convert()'s correctly-aliased one.

Root-caused via RVC3-python's chap11.ipynb motion-detection example:
VideoFile("...", mono=True, dtype='float') produced float64 frames,
and arithmetic on them stayed float64 all the way to
Image.disp(matplotlib=False), where OpenCV's cvtColor doesn't support
CV_64F -- 'Unsupported depth of input image'.

Fix: hoisted the alias table to a single shared constant,
machinevisiontoolbox.base.types.DTYPE_ALIASES, used by both convert()
and _infer_dtype. Documented the aliases via one Sphinx rst_epilog
substitution (|dtype_aliases|, matching the existing |RVC3| pattern
and bdsim's |BlockOptions| convention) referenced from both
docstrings, rather than duplicating the explanation.

Since every ImageSource (VideoFile, VideoCamera, ImageCollection, ...)
constructs Image instances with a dtype= option, this affects all of
them uniformly, not just VideoFile.

Added tests/test_dtype_resolution.py: a single parametrized matrix
(every DTYPE_ALIASES case x every dtype-resolving entry point) rather
than scattered one-off tests per bug. Three independent, inconsistent
dtype resolvers turned out to exist across this codebase (this one,
plus Image.to()/.array_as()/.astype(), plus the ImageConstantsMixin
factory methods -- both to be fixed on separate follow-up branches);
this module is designed to grow a test method per entry point as each
one is fixed, so the whole class of bug is pinned down in one place
instead of drifting silently again.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@codacy-production

Copy link
Copy Markdown

Not up to standards ⛔

🔴 Issues 2 high

Alerts:
⚠ 2 issues (≤ 0 issues of at least minor severity)

Results:
2 new issues

Category Results
Security 2 high

View in Codacy

🟢 Metrics 0 complexity · 0 duplication

Metric Results
Complexity 0
Duplication 0

View in Codacy

NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant