Skip to content

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

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

Conversation

trygvrad
Copy link
Contributor

@trygvrad trygvrad commented Apr 6, 2025

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:

  • A MultiNorm class. This is a subclass of colors.Normalize and holds n_variate norms.
  • Testing of the MultiNorm class

Features not included in this PR:

  • changes to colorizer.py needed to expose the MultiNorm class
  • Exposes the functionality provided by MultiNorm together with BivarColormap and MultivarColormap to the plotting functions axes.imshow(...), axes.pcolor, and `axes.pcolormesh(...)
  • Testing of the new plotting methods
  • Examples in the docs

Comment on lines 4103 to 4105
in the case where an invalid string is used. This cannot use
`_api.check_getitem()`, because the norm keyword accepts arguments
other than strings.
Copy link
Member

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).

Copy link
Contributor Author

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 .

@trygvrad trygvrad force-pushed the multivariate-plot-prapare branch from 55b85e3 to f42d65b Compare April 17, 2025 15:18
Copy link
Contributor

@anntzer anntzer left a 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?

@trygvrad
Copy link
Contributor Author

trygvrad commented May 4, 2025

Thank you for the feedback @anntzer !
Hopefully we can hear if @timhoffm has any thoughts on n_input / input_dims naming within the coming week.

@timhoffm
Copy link
Member

timhoffm commented May 5, 2025

See #29876 (comment)

@trygvrad
Copy link
Contributor Author

trygvrad commented May 7, 2025

Thank you @timhoffm
The PR should now be as we agreed (#29876 (comment)) :)

@trygvrad
Copy link
Contributor Author

trygvrad commented Jun 1, 2025

@QuLogic Thank you again and apologies for my tardiness (I was sick)
@timhoffm Do you think you could approve this PR now?

@trygvrad trygvrad force-pushed the multivariate-plot-prapare branch from 6b86d63 to 32247f5 Compare June 4, 2025 21:01
@trygvrad
Copy link
Contributor Author

trygvrad commented Jun 6, 2025

This is on hold until we sort out #30149 (Norm Protocol)
see #29876 (comment)

@trygvrad trygvrad force-pushed the multivariate-plot-prapare branch 3 times, most recently from dd8f0c6 to 67b8264 Compare July 13, 2025 11:56
trygvrad and others added 4 commits July 20, 2025 16:05
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>
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
@trygvrad trygvrad force-pushed the multivariate-plot-prapare branch from 2707405 to da1ac73 Compare July 24, 2025 20:40
Copy link
Member

@timhoffm timhoffm left a 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. :)

trygvrad and others added 2 commits July 29, 2025 19:46
Co-authored-by: Tim Hoffmann <2836374+timhoffm@users.noreply.github.com>
@trygvrad trygvrad force-pushed the multivariate-plot-prapare branch from 4f72962 to 81372c6 Compare July 29, 2025 17:54
@trygvrad
Copy link
Contributor Author

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

Copy link
Member

@timhoffm timhoffm left a 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.
Copy link
Member

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.

Copy link
Contributor Author

@trygvrad trygvrad Aug 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@timhoffm Do you agree with @QuLogic that we should allow a single norm?
I'm fine with it either way

Copy link
Member

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.

Comment on lines 3446 to 3447
elif not np.iterable(clip):
clip = [clip]*self.n_components
Copy link
Member

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?

@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]: ...
Copy link
Member

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, ...])?

Copy link
Contributor Author

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):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this unused?

Comment on lines +4316 to +4322
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
Copy link
Member

@QuLogic QuLogic Jul 30, 2025

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,

Suggested change
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.

Copy link
Contributor Author

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»

Copy link
Member

@QuLogic QuLogic Jul 31, 2025

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.

trygvrad and others added 3 commits August 1, 2025 12:54
@trygvrad
Copy link
Contributor Author

trygvrad commented Aug 1, 2025

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.

@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}")
Copy link
Member

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

Suggested change
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

@anntzer
Copy link
Contributor

anntzer commented Aug 1, 2025

@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 :)

Sorry I dropped the ball on this. I can try to have a look but not before the middle of the month or so.

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

Successfully merging this pull request may close these issues.

Feature request: Bivariate colormapping
7 participants