-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
MultiNorm class #29876
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
MultiNorm class #29876
Conversation
lib/matplotlib/colors.py
Outdated
in the case where an invalid string is used. This cannot use | ||
`_api.check_getitem()`, because the norm keyword accepts arguments | ||
other than strings. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm confused because this function is only called for isinstance(norm, str)
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So this function exists because the norm keyword accepts Normalize
objects in addition to strings.
This is fundamentally the same error you get if you give an invalid norm to a Colorizer
object.
In main, the @norm.setter
on colorizer.Colorizer
reads:
@norm.setter
def norm(self, norm):
_api.check_isinstance((colors.Normalize, str, None), norm=norm)
if norm is None:
norm = colors.Normalize()
elif isinstance(norm, str):
try:
scale_cls = scale._scale_mapping[norm]
except KeyError:
raise ValueError(
"Invalid norm str name; the following values are "
f"supported: {', '.join(scale._scale_mapping)}"
) from None
norm = _auto_norm_from_scale(scale_cls)()
...
The _get_scale_cls_from_str()
exists in this PR because this functionality is now needed by both colorizer.Colorizer.norm()
and colors.MultiNorm
.
Note this PR does not include changes to colorizer.Colorizer.norm()
so that it makes use of _get_scale_cls_from_str()
. These changes follow in the next PR: #29877 .
55b85e3
to
f42d65b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some minor points, plus re-pinging @timhoffm in case he has an opinion re: n_input / input_dims naming?
See #29876 (comment) |
Thank you @timhoffm |
6b86d63
to
32247f5
Compare
This is on hold until we sort out #30149 (Norm Protocol) |
dd8f0c6
to
67b8264
Compare
This commit merges a number of commits now contained in https://github.com/trygvrad/matplotlib/tree/multivariate-plot-prapare-backup , keeping only the MultiNorm class
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
81ab0d5
to
babbee0
Compare
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
2707405
to
da1ac73
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @trygvrad! I think we're almost there. You can see the progress in moving from architecture and design questions to docs. :)
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
4f72962
to
81372c6
Compare
I changed vmin, vmax and clip to only accept iterables or None. It dawned on me that it will be useful to allow None, with the behavior that it does not modify the vmin/vmax/clip on the constituent norms. This will be useful for use as a default, but also be useful for users that want to combine multiple norms into a MultiNorm without modifying the limits of the norms. i.e. n2 = mpl.colors.Normalize(vmax=2)
n3 = mpl.colors.Normalize(vmax=3)
mn0 = mpl.colors.MultiNorm((n2, n3), vmax=None) # does not modify vmax on the constituent norms
print(mn0.vmax, n2.vmax, n3.vmax) # prints (2.0, 3.0) 2.0 3.0
mn0 = mpl.colors.MultiNorm((n2, n3), vmax=[None, 4]) # vmax will now be None on n2 and 4 on n3
print(mn0.vmax, n2.vmax, n3.vmax) # prints (None, 4.0) None 4.0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @trygvrad for the patience and effort to go through all the design decisions together!
Parameters | ||
---------- | ||
norms : list of (str or `Normalize`) | ||
The constituent norms. The list must have a minimum length of 2. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand 0, but is there a reason we can't gracefully degrade to 1D-Normalize
behaviour if passed only 1?
I'm thinking of someone doing MultiNorm(norms)
where norms
is some dynamically deduced thing, and having to special-case 1 seems annoying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see a practical use case for 1-element. This "dynamically deducing" seems a bit unlikely. But OTOH there's also no benefit in reqiring at least 2. So a list of length 1 is ok to accept.
lib/matplotlib/colors.py
Outdated
elif not np.iterable(clip): | ||
clip = [clip]*self.n_components |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use np.broadcast_to
as well?
lib/matplotlib/colors.pyi
Outdated
@overload | ||
def __call__(self, values: tuple[np.ndarray], clip: ArrayLike | bool | None = ...) -> tuple[np.ndarray]: ... | ||
@overload | ||
def __call__(self, values: tuple[float], clip: ArrayLike | bool | None = ...) -> tuple[float]: ... |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this really supposed to be a tuple of a single float? Or a single float (float
)? Or a tuple of floats (tuple[float, ...]
)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be tuple of floats, I'll fix it. Thanks for spotting it
|
||
|
||
@staticmethod | ||
def _ensure_multicomponent_data(data, n_components): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this unused?
try: | ||
scale_cls = scale._scale_mapping[scale_as_str] | ||
except KeyError: | ||
raise ValueError( | ||
f"Invalid norm name {scale_as_str!r}; supported values are " | ||
f"{', '.join(scale._scale_mapping)}" | ||
) from None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use _api.check_getitem
which automatically chooses the right keys,
try: | |
scale_cls = scale._scale_mapping[scale_as_str] | |
except KeyError: | |
raise ValueError( | |
f"Invalid norm name {scale_as_str!r}; supported values are " | |
f"{', '.join(scale._scale_mapping)}" | |
) from None | |
scale_cls = _api.check_getitem(scale._scale_mapping, norm=scale_as_str) |
should work.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There used to be a comment stating why api.check_getiem is not used here, but not sure where it went.
In any case, the reason is that the error message produced by api.check_getitem indicates that only stings are valid for the norm keyword (I.e in imshow). In reality, both strings and Normalize objects are valid, which is why the error message here specifies «invalid norm name»
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But check_getitem
doesn't say anything about strings, just the keys from the mapping you give it, and this current exception here doesn't say anything about Normalize
objects either, just the keys from the mapping.
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
@timhoffm @anntzer I had made these changes, but actually committing them had slipped my mind. They are in now 😅 I'm on vacation now, so my schedule is a bit scrambled. @QuLogic I'll take a look at the rest of your comments soon :) |
return | ||
if not np.iterable(values) or len(values) != self.n_components: | ||
raise ValueError("*vmin* must have one component for each norm. " | ||
f"Expected an iterable of length {self.n_components}") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's always good to show the offending value for context
f"Expected an iterable of length {self.n_components}") | |
f"Expected an iterable of length {self.n_components}, " | |
f"but got {values!r}") |
Also in all similar error messages
Sorry I dropped the ball on this. I can try to have a look but not before the middle of the month or so. |
PR summary
This PR continues the work of #28658 and #28454, aiming to close #14168. (Feature request: Bivariate colormapping)
This is part one of the former PR, #29221. Please see #29221 for the previous discussion
Features included in this PR:
MultiNorm
class. This is a subclass ofcolors.Normalize
and holdsn_variate
norms.MultiNorm
classFeatures not included in this PR:
MultiNorm
together withBivarColormap
andMultivarColormap
to the plotting functionsaxes.imshow(...)
,axes.pcolor
, and `axes.pcolormesh(...)