fix: ImageConstantsMixin factories silently downcast explicit dtype requests - #88
Open
petercorke wants to merge 1 commit into
Open
Conversation
…equests
A third, distinct dtype bug from the constructor/convert() alias
mismatch this branch is based on: Image.Zeros()/.Constant()/.Random()/
.Squares()/.Circles()/.Ramp()/.Sin()/.Chequerboard()/.Polygons() all
build their raw pixel array with the caller's dtype, but never forward
dtype= to the final Image(...) constructor call. Since dtype=None
triggers the constructor's own auto-detect ("no dtype given -> any
floating input becomes float32"), even a fully-correct, explicit
dtype='float64' request was silently downcast to float32 --
Image.Zeros(size=5, dtype='float64').dtype was float32.
Two fixes:
- _resolve_pattern_options() (shared by all 9 factories above) now
resolves DTYPE_ALIASES itself, uniformly regardless of whether dtype
came from the caller's own argument or a default_dtype/like.dtype
fallback -- Constant()'s default_dtype='float' for float values
needed this too, not just the explicit-argument path.
- Each factory's final constructor call now passes dtype=True ("trust
the array I already built, don't second-guess it") instead of
omitting dtype= entirely. Squares/Circles/Ramp/Sin/Chequerboard/
Polygons share a _pattern_image() helper -- fixed once there rather
than at each of the 6 call sites.
String() was already fine -- it forwards **kwargs straight to the
(already-fixed) constructor rather than handling dtype itself.
Extended tests/test_dtype_resolution.py with a factory x dtype matrix
(10 factories x 8 dtype cases = 80 new cases) rather than one-off
tests per factory. Verified against pre-fix code: 45 of the 80 fail
(the cases where a factory's own default dtype doesn't coincidentally
already match the auto-detect outcome), all pass with the fix.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Up to standards ✅🟢 Issues
|
| 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.
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_ALIASES). A third, distinct dtype bug found while auditing every dtype-resolving entry point (see #86, #87):Image.Zeros()/.Constant()/.Random()/.Squares()/.Circles()/.Ramp()/.Sin()/.Chequerboard()/.Polygons()all build their raw pixel array with the caller'sdtype, but never forwarddtype=to the finalImage(...)constructor call. Sincedtype=Nonetriggers the constructor's own auto-detect ("no dtype given -> any floating input becomesfloat32"), even a fully-correct, explicitdtype='float64'request was silently downcast tofloat32--Image.Zeros(size=5, dtype='float64').dtypewasfloat32.Two fixes:
_resolve_pattern_options()(shared by all 9 factories above) now resolvesDTYPE_ALIASESitself, uniformly regardless of whetherdtypecame from the caller's own argument or adefault_dtype/like.dtypefallback --Constant()'sdefault_dtype='float'for float values needed this too, not just the explicit-argument path.dtype=True("trust the array I already built, don't second-guess it") instead of omittingdtype=entirely.Squares/Circles/Ramp/Sin/Chequerboard/Polygonsshare a_pattern_image()helper -- fixed once there rather than at each of the 6 call sites.String()was already fine -- it forwards**kwargsstraight to the (already-fixed) constructor rather than handlingdtypeitself.Test plan
tests/test_dtype_resolution.pywith a factory x dtype matrix (10 factories x 8 dtype cases = 80 new cases) rather than one-off tests per factorytests/test_image_constants.py+tests/test_dtype_resolution.py+tests/test_image_core.pysuites pass (189 passed)