Fix misleading cmap errors when norm/palette interact#653
Merged
Conversation
timtreis
force-pushed
the
fix/issue-622-norm-cmap-palette-errors
branch
from
e0cc677 to
a5dfd13
Compare
Three related defects all manifested as errors blaming `cmap` for a `norm`/`palette` mistake the user never made: 1. `norm=[n1, n2]` for a 3-channel image raised "If 'cmap' is provided, its length must match the number of channels." even though the user only passed `norm=`. _validate_image_render_params now validates the norm-list length against the channel count up front, with a norm-specific error message. 2. `cmap=["Reds","Greens"]` (wrong length) combined with `norm=` of correct length silently nulled the cmap and used the default. The wrong-length cmap is now rejected uniformly, matching the no-norm path. 3. `palette + norm=list` raised "If 'palette' is provided, 'cmap' must be None." because basic.py auto-expanded a default-cmap list to carry the per-channel norms, and render.py couldn't tell the synthesized list apart from a user-supplied one. The render path now keys the palette/cmap conflict on whether any CmapParams was actually user-supplied (cmap_is_default=False); the synthesized default-cmap list falls through to the palette path, where per-channel norms are already applied to the layers. The "blending multiple cmaps" warning is also gated on user-supplied cmaps, so it no longer fires spuriously when the user only set `norm` or `palette + norm=list`.
timtreis
force-pushed
the
fix/issue-622-norm-cmap-palette-errors
branch
from
a5dfd13 to
8ddfffa
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #653 +/- ##
==========================================
+ Coverage 76.89% 76.99% +0.09%
==========================================
Files 11 11
Lines 3277 3282 +5
Branches 774 775 +1
==========================================
+ Hits 2520 2527 +7
+ Misses 457 455 -2
Partials 300 300
🚀 New features to boost your workflow:
|
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
2 participants
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.
Closes #622.
Summary
Three related defects all surfaced as errors blaming
cmapfor anorm/palettemistake the user never made:norm=[n1, n2]for a 3-channel image raised"If 'cmap' is provided, its length must match the number of channels."even though the user only passednorm=.cmap=[...]of wrong length combined withnorm=of correct length silently nulled the cmap and rendered with the default — asymmetric with the no-norm path which raises.palette=[...] + norm=[...](per-channel norms) raised"If 'palette' is provided, 'cmap' must be None."even though the user never passedcmap.Fixes
utils.py::_validate_image_render_params: validatenorm-list length against the channel count up front with anorm-specific error message; reject wrong-lengthcmapinstead of silently nulling it.render.py: distinguish user-supplied multi-cmaps from the default-cmap list synthesized bybasic.pyto carry per-channel norms (viaCmapParams.cmap_is_default). The palette/cmap conflict and the "blending multiple cmaps" warning now fire only when at least one cmap is user-supplied. Thepalette + norm=listcase falls through to the palette path; per-channel norms are already applied to the layers in the per-channel norm loop.