fix: to()/array_as() same dtype-alias bug as the constructor; astype() double-cast - #87
Open
petercorke wants to merge 1 commit into
Open
Conversation
…) double-cast
Same root cause as the constructor/convert() fix this branch is based
on: Image.to()/.array_as() called plain np.dtype(dtype) with no
DTYPE_ALIASES lookup, so dtype='float'/'int' resolved to NumPy's own
float64/platform-int rather than the Toolbox's float32/uint8
convention. 'double' and 'half' never showed this bug -- NumPy's own
np.dtype('double')/('half') already equal float64/float16, coincidentally
matching DTYPE_ALIASES for those two.
to() also did its own separate premature dtype = np.dtype(dtype) before
calling array_as(dtype) -- simplified to just pass dtype through
unchanged now that array_as() and the Image constructor both resolve
it themselves; no need to resolve it three times.
astype() had no test-visible bug (its final output was already correct,
self-corrected by the constructor's now-fixed dtype= handling in
self.__class__(self._A.astype(dtype), dtype=dtype)) but did an
unnecessary intermediate cast to NumPy's raw (wrong) interpretation of
the alias before the constructor cast it again to the right one --
e.g. astype('float') cast uint8->float64->float32 instead of directly
uint8->float32. Resolved the alias before the first cast to avoid the
redundant round-trip.
Extended tests/test_dtype_resolution.py's shared parametrized matrix
with test methods for these three entry points, rather than new
one-off tests -- confirms to()/array_as() fail against pre-fix code
for the 'int'/'float' cases specifically (not 'double'/'half', matching
the analysis above), and that astype() has no output-level regression
to catch.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Security | 3 high |
🟢 Metrics 0 complexity · 0 duplication
Metric Results Complexity 0 Duplication 0
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.
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Stacked on #86 (needs
DTYPE_ALIASESfrom that branch). Same root cause found while auditing every dtype-resolving entry point after #86:Image.to()/Image.array_as()both called plainnp.dtype(dtype)with noDTYPE_ALIASESlookup, sodtype='float'/'int'resolved to NumPy's ownfloat64/platform-int rather than the Toolbox'sfloat32/uint8convention.'double'and'half'never showed this bug -- NumPy's ownnp.dtype('double')/('half')already equalfloat64/float16, coincidentally matchingDTYPE_ALIASESfor those two.to()also did its own separate prematuredtype = np.dtype(dtype)before callingarray_as(dtype)-- simplified to just passdtypethrough unchanged now thatarray_as()and theImageconstructor both resolve it themselves.astype()had no test-visible bug -- its final output was already correct, self-corrected by the constructor's now-fixeddtype=handling inself.__class__(self._A.astype(dtype), dtype=dtype)-- but did an unnecessary intermediate cast to NumPy's raw (wrong) interpretation of the alias before the constructor cast it again to the right one, e.g.astype('float')castuint8->float64->float32instead of directlyuint8->float32. Resolved the alias before the first cast to avoid the redundant round-trip.Test plan
tests/test_dtype_resolution.py's shared parametrized matrix with test methods forto()/array_as()/astype(), rather than new one-off teststo()/array_as()fail for exactly the'int'/'float'cases (not'double'/'half', matching the analysis above);astype()correctly shows no output-level regressiontests/test_image_core.py+tests/test_dtype_resolution.pysuites pass (119 passed)