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 8 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

This commit introduces the MultiNorm calss to prepare for the introduction of multivariate plotting methods
@@ -2320,6 +2320,16 @@ def __init__(self, vmin=None, vmax=None, clip=False):
self._scale = None
self.callbacks = cbook.CallbackRegistry(signals=["changed"])

@property
def n_input(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps input_dims, output_dims for consistency with Transforms?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dimensions is probably somewhat confusing term in this context, because it typically means the number of axes in an array, i.e. np.zeros((4,5,6)) has three dimensions, but the corresponding n_input would be four [if shown as an image of size (5,6)].

What do you think of input_variates and output_variates, or input_vars and output_vars?

Copy link
Contributor

@anntzer anntzer Apr 7, 2025

Choose a reason for hiding this comment

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

Sure, although transforms use the same terminology? No strong opinion there.
However this does raise another issue, which is that you seem to put the variates as the first dimension. Intuitively I'd rather put them as the last dimension (just like if you pass a 2D array to plot, the last dimension are the different variates). I'm sure this design choice must have been discussed somewhere, can you link that discussion?

Copy link
Contributor Author

@trygvrad trygvrad Apr 7, 2025

Choose a reason for hiding this comment

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

We had a brief discussion on this here #14168 (comment), but we followed that up on a weekly meeting 14 September 2023 https://hackmd.io/@matplotlib/Skaz1lrh6#Mutlivariate-colormapping .

The argument is not formulated well in the log, but basically (V, N, M) because there are V cmaps and norms, and in this way the input for the data mirrors that of the vmin, vmax and norm keywords, i.e.:
ax.imshow((data0, data1), vmin=(vmin0, vmin1), cmap='BiOrangeBlue')
or

ax.imshow((preassure, temperature), 
           vmin=(vmin_preassure, vmin_temperature), 
           cmap='BiOrangeBlue')

Quite often the data is qualitatively different, like the example at the bottom of the page here: https://trygvrad.github.io/mpl_docs/Multivariate%20colormaps.html where GDP_per_capita and Annual_growth are plotted together. I find it is not very intuitive to wrap these in a single array.

(also: this allows the different variates to have different data types)

Copy link
Contributor

@anntzer anntzer Apr 10, 2025

Choose a reason for hiding this comment

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

Sure, looks like I already lost that discussion once so let's not relitigate it :)
I'll just ping @timhoffm who may have some idea as to the best name for n_input/input_dims (or some other name).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @timhoffm :D

Why do we have to distinguish input and output? They are both the same here. Do you anticipate that there can be norms with different inputs and outputs? What would be an example for that? - I'm asking because that decides whether we need the input/output parts in the name.

The thinking was that there is no requirement that they be equal, so as to be future-proof they are here implemented as separate. I do believe this was mentioned at a meeting at one point, but I have spent the last day trying to imagine a use case where it would be beneficial to have them separate. I can imagine some use cases where one might want to map a single scalar to two colorbars, such as height above and below sea level on separate colorbars, but even as I stretch my mind to find a use case where one scalar maps to multiple colorbars, I cannot find a use case where it is more intuitive to integrate that into the norm than to handle the additional complexity a separate pre-processing step and use a norm where the number of inputs and outputs are the same.

With that in mind I think we should change the names from n_input/n_output to a single variable name.

In my mind the following are good options:

  • n_variates
  • n_channels
  • n_dims
    Or we could go even shorted and just use variates, channels or dims.

I have no strong opinion now, but this has been my logic on this topic previously:
When n_variates was implemented in MultivariateColormap my thinking was that we have the ability to define our own vocabulary in this case, and whatever word we choose, it is better if we are consistent, thus we ended up with MultivariateColormap and n_variates, but we could alternatively have MultichannelColormap and n_channels. If we choose to use n_variates also for MultiNorm it would give an indication to that this relates to MultivariateColormap. Similarly, the word 'dims' is used in the context of transform, while the word 'channel' is weakly connected to the concept of an alpha-channel. My hunch has been that using a less-used word [variates] gives us more power to imbue that word with meaning, and that makes it easier to be specific in our vocabulary going forward.

How often will these properties be used by end users? - I'm asking, because if they are rarely used, we may affort longer names for more clarity.

I don't intend for users to use them. The number is implicit from the creation of a MultiNorm, i.e. MultiNorm(['log', None]) or even plt.imshow([a, b], cmap='2AddA', norm=['log', None]).

Their importance is only for error-handling, to make sure that the data, norm and colormap are compatible, and give the user a reasonable error.

@anntzer I do not hold strong opinion, so if you let me know what you prefer, I will update this PR accordingly.
As this is not something we expect users to interact with much, the main concern should be the maintainers :)

Copy link
Member

Choose a reason for hiding this comment

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

  1. I agree that a single variable is sufficient. It's not the responsibility of the Norm to create or condense information. So it can only be N -> N. It could possibly even be 1 -> 1 if we allow a list of Norm instances for the N -> N case. Advantage would be that we don't need to create any new norms and the logic stays simpler. The only thing we'd loose is the ability to couple the variables, but I don't see that that's needed. - We could always later introduce MultiNorm. To be checked: Maybe it's favorable internally to collect these into a single instance, but that would not need to be public. What do you think?

  1. I asked ChatGPT "would you use variate as a standalone term?":

Excellent question — and honestly, "variate" isn’t super commonly used by itself in casual conversation or even in a lot of applied work, but it does exist as a proper term in stats.

Technically:

  • A variate is a random variable, or a measurable quantity that can take on different values.
    Example: If you're studying people's heights, each person’s height would be a value of the "height variate."

In practice:

  • You’re way more likely to hear "variable" or "random variable" than "variate."
  • "Variate" mostly shows up in more formal, old-school, or academic statistical texts.
    Example: "The sample contains 100 observations of the variate X."

It really shines in compound terms like:

  • Univariate → one variable
  • Bivariate → two variables
  • Multivariate → multiple variables

But you wouldn’t usually say, “Let’s analyze this variate.” You’d just say, “variable.”

  1. I don't intend for users to use n_variates. Then (if we still want to expose MultiNorm), let's just call this n_variables and keep it private.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Lets go for n_variables then, it's a good name :)

So it can only be N -> N. It could possibly even be 1 -> 1 if we allow a list of Norm instances for the N -> N case. Advantage would be that we don't need to create any new norms and the logic stays simpler. The only thing we'd loose is the ability to couple the variables, but I don't see that that's needed. - We could always later introduce MultiNorm. To be checked: Maybe it's favorable internally to collect these into a single instance, but that would not need to be public. What do you think?

We have discussed this before in #28428 where I initially implemented multivariate color mapping with list of norms instead of MultiNorm #28428 (comment) . This was then later changed to MultiNorm.

However, this was before the introduction of the Colorizer as a container for norm→colormap.

The choice of having a list of norms or a MultiNorm object is now somewhat a question of where we put the additional complexity for supporting norms for multivariate/bivariate colormaps. Do we put the complexity in A: Colorizer or B: MultiNorm. My hypothesis is that having MultiNorm as a separate class simplifies the colorizer class, as we can call methods on the Colorizer.norm regardless of if the data is multivariate or not. In my mind this will make it easier to maintain.

The top-level plotting functions should be largely unaffected by this choice, as they should operate through the colorizer interface, and should not use the norm directly.

If we choose to have a MultiNorm class, it is then a separate question if MultiNorm should be private. To me, this boils down to what should the user recieve if requesting the norm after plotting a bivariate/multivariate image:

im = plt.imshow((a, b), cmap='BiOrangeBlue')
im.norm # ← what should this be?

My hypothesis here has been that it is easier for the user ìm.norm has type stability, i.e. it is always a subclass of colors.Normalize, and therefore we make MultiNorm public.

@timhoffm Let me know if you want me to implement a version with a list of norms instead of MultiNorm, it will have to be prototyped as a complete solution (with working top-level plotting methods) similar to #29221 , but if we go for that solution I could then break it into smaller PRs for review.

Copy link
Member

Choose a reason for hiding this comment

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

Well, you don't really have type stability. While MultiNorm is formally a subclass of Normalize, it violates LSP (*), the call you need to do on im.norm depends on the type.

(*) I'm not a LSP nazi, it's sometimes ok to violate it. But you can't argue for that with type stability.

My hypothesis is that having MultiNorm as a separate class simplifies the colorizer class

This is a valid argument and we should at least have MultiNorm internally.

To me, this boils down to what should the user recieve if requesting the norm after plotting a bivariate/multivariate image

Yes. Technically that's a separate question, and I don't have a strong preference here. But since the class exist, it may be ok to expose it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

All true, I was a bit inaccurate in my comment.
I have changed from n_input/n_output to n_variables as agreed :)

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 .

Thank you @QuLogic

Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
@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?

@@ -3219,6 +3224,224 @@ def inverse(self, value):
return value


class MultiNorm(Normalize):
"""
A mixin class which contains multiple scalar norms
Copy link
Member

Choose a reason for hiding this comment

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

AFAICS, this is not a mixin. It's a "normal" subclass.

Speaking of which, have we discussed whether we want to make this a subclass of Normalize? This breaks LSP, not that I'm a hardcore LSP advocate, but I feel we may become unnecessarily sloppy.

It may be reasonable to introduce a common Norm base class (or possibly just a protocol). Which both Normalize and MultiNorm (should we call this MultiNormalize?) derive from/implement.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@timhoffm I don't think we have discussed this, but maybe we should. What solution do you think is preferable?
@story645 Should we add this a topic for the next weekly meeting?

Copy link
Member

@timhoffm timhoffm Jun 4, 2025

Choose a reason for hiding this comment

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

I'd feel more comfortable if MultiNorm is not a subclass of Normalize. Normalize is already weird enough in that the linear norm is the base class for all other (scalar) norms. Having a generic Norm concept. Makes sense to me. I've not looked in to the implementation to judge whether abstract base class or protocol is the better implementation. - Unfortunately, I cannot predict whether I can make it to the next weekly meeting.

Copy link
Contributor Author

@trygvrad trygvrad Jun 5, 2025

Choose a reason for hiding this comment

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

To me an abstract base class seems more intuitive than a protocol, but I have no strong preference.
(I find one use of an abstract class in the extisting codebase (animation.AbstractMovieWriter), and no use of a Protocol)

In that case I have some questions:

  1. Should it only have methods with @abstractmethod or also include some implementations (I'm thinking of @property def n_variables(self) and def _changed(self).
  2. There are a lot of required functions for a norm so that it can function in all cases, vmin, vmax, inverse, autoscale, autoscale_none, scaled, clip should the abstract class define all of them with @abstractmethod, or only a subset.
  3. I assume we will need to change the docstrings for Colorizer immediately. Once the top level functions can accept a MultiNorm, we will have to change the docstrings here as well, to specifiy that input can be Norm instead of Normalize

I'm thinking something along these lines:

class Norm(ABC):

    @property
    def n_variables(self):
        # To be overridden by subclasses with multiple inputs
        return 1

    def _changed(self):
        """
        Call this whenever the norm is changed to notify all the
        callback listeners to the 'changed' signal.
        """
        self.callbacks.process('changed')

    @property
    @abstractmethod
    def vmin(self):
        pass

    @property
    @abstractmethod
    def vmax(self):
        pass

    @property
    @abstractmethod
    def clip(self):
        return self._clip


    @abstractmethod
    def __call__(self, value, clip=None):
        pass

    @abstractmethod
    def inverse(self, value):
        pass

    def autoscale(self, A):
        pass

    def autoscale_None(self, A):
        pass

    @abstractmethod
    def scaled(self):
        pass

I'm somewhat tempted to write it in such a way that NoNorm could in theory inherit only from Norm and not from Normalize. I don't think there is any practical reason to do so, but if I was designing the architecture from scratch I would have made at least Normalize, NoNorm and MultiNorm inherit from the abstract class Norm.

Copy link
Member

Choose a reason for hiding this comment

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

(I find one use of an abstract class in the extisting codebase (animation.AbstractMovieWriter), and no use of a Protocol)

We have several base classe (FigureBase, _ImageBase, TransformNode, ...`) that are not technically abstract, but mainly define API and common functionality but are not meaningful on their own.

There are no Protocols in Matplotlib because Protocols are much younger than Matplotlib and on top, they are only really helpful if you use typing, which we have just adopted.

IMHO the decision between abstract class and protocol should be taken on how much common code and (internal) logic there is. A protocol is less invasive because you can keep the Normalize class exactly as is and just define a compatible standalone MultiNorm next to it. The compatibility is ensured through adding the protocol. From your code example it looks like only _changed() and clip are duplicated, which I find small enough to justify a protocol. But no strong opinion either.

In case of going with an abstract class, it'd be advisable to try and introduce this frist in a separate PR.

To the questions:

  1. There's no real value in pure abstract classes (in contrast, that would rather point towards a protocol). If you have shared functionality, that can be implemented in the abstract base class.
  2. The abstract class should define the complete common API such that some def f(norm: Norm): norm.somefunc will not be flagged by type checkers.
  3. Yes.
  4. I think I would not bother with NoNorm right now. It's working as is. While it's logically a bit awkward that it derives from Normalize, the same holds true for all the non-linear norms. Additionally, putting NoNorm directly under Norm would require that you implement all the abstract methods / the complete protocol.

Copy link
Contributor Author

@trygvrad trygvrad Jun 5, 2025

Choose a reason for hiding this comment

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

@tacaswell expressed a preference for a Protocol. The consensus in the meeting was that this will prevent it from growing into its own beast.

I will try to make this in a separate branch, and tag you @timhoffm. I will probably need some feedback on how to document it.

EDIT: the new PR is in #30149

vmin, vmax : float, None, or list of float or None
Limits of the constituent norms.
If a list, each value is assigned to each of the constituent
norms. Single values are repeated to form a list of appropriate size.
Copy link
Member

Choose a reason for hiding this comment

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

Is broadcasting reasonable here? I would assume that most MultiNorms have different scales and thus need per-element entries anyway. It could also be an oversight to pass a single value instead of multiple values.

I'm therefore tempted to not allow scalars here but require exactly n_variables values. A more narrow and explicit interface may be the better start. We can always later expand the API to broadcast scalars if we see that's a typical case and reasonable in terms of usability.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@timhoffm Perhaps this is also a topic for the weekly meeting :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm perfectly fine with removing this here, and perhaps that is a good starting point.

My entry into this topic was a use case (dark-field X-ray microscopy, DFXRM) where we typically want vmax0 = vmax1 = -vmin0 =-vmin1, i.e. equal normalizations, and centered on zero, and given that entry point it felt natural to me to include broadcasting.

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

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
6 participants