Repository navigation
Creation of the Norm Protocol - #30149
trygvrad wants to merge 2 commits into
Conversation
Sorry, something went wrong.
| class Norm(Protocol): | ||
| callbacks: cbook.CallbackRegistry | ||
| @property | ||
| def vmin(self) -> float | None: ... |
There was a problem hiding this comment.
Wouldn't we need something like this to support MultiNorm?
| def vmin(self) -> float | None: ... | |
| def vmin(self) -> float | ArrayLike | None: ... |
Sorry, something went wrong.
There was a problem hiding this comment.
Yes,
My logic was to get this into main first, and then change this with the MultiNorm PR, but I guess it is easier if I do it here. I will try to find the time early next week.
Is this PR otherwise as you would expect?
Sorry, something went wrong.
There was a problem hiding this comment.
This should be updated now, I set it to:
@property
def vmin(self) -> float | tuple[float] | None: ...
@property
def vmax(self) -> float | tuple[float] | None: ...
@property
def clip(self) -> bool | tuple[bool]: ...
Sorry, something went wrong.
|
Considering this again, I'm leaning slightly towards an abstract base class. To be clear, both variants are acceptable solutions for the use case and would do the job. The relevant arguments for me are:
Since I missed the discussion: What were the reasons to suggest unsung Protocols? |
Sorry, something went wrong.
|
The main reasoning to avoid ABCs was to avoid having to deal with metaclasses, since that is often more painful than helpful. Protocols get us to essentially the same place without having to worry about that aspect. |
Sorry, something went wrong.
|
@trygvrad Thanks for taking the effort to write out both alternatives! I still like the ABC approach a bit more. Could you please add Norm to the docs here to see how that renders. @ksunden I think ABC is much simpler than a generic metaclass. The only complication I see is that one would have to watch out in case of multiple inheritance - But I don't see a case where norms would need multiple inheritance. They are quite straight forward. |
Sorry, something went wrong.
|
@timhoffm I'm not able to work on this now, but I will try to get to it at the end of the week. @tacaswell If i recall you had a preference for a Protocol at the weekly meeting. What is your opinion now, in light of @timhoffm arguments here #30149 (comment) ? |
Sorry, something went wrong.
|
Closing in favor of #30178 (see #30178 (comment)) Thank you @trygvrad for working up both versions! |
Sorry, something went wrong.
This PR is a response to a discussion in the past weeks weekly developer meeting regarding the creation of a Norm protocol before the introduction of MultiNorm #29876 (comment)
Prior to this PR there are no Protocols in matplotlib.
This implementation uses @runtime_checkable so that Colorizer.set_norm() can check _api.check_isinstance((colors.Norm, str, None), norm=norm)
Note that the error message if the class one attempts to use is missing a member, is just the standard wrong-type message, and does not tell you what member of the protocol is missing.
@timhoffm @tacaswell @ksunden @story645
The implementation looks like this:
@runtime_checkable class Norm(Protocol): @property def vmin(self): """Lower limit of the input data interval; maps to 0.""" ... @property def vmax(self): """Upper limit of the input data interval; maps to 1.""" ... @property def clip(self): """ Determines the behavior for mapping values outside the range ``[vmin, vmax]``. See the *clip* parameter in `.Normalize`. """ ... def _changed(self): """ Call this whenever the norm is changed to notify all the callback listeners to the 'changed' signal. """ ... def __call__(self, value, clip=None): """ Normalize the data and return the normalized data. Parameters ---------- value Data to normalize. clip : bool, optional See the description of the parameter *clip* in `.Normalize`. If ``None``, defaults to ``self.clip`` (which defaults to ``False``). Notes ----- If not already initialized, ``self.vmin`` and ``self.vmax`` are initialized using ``self.autoscale_None(value)``. """ ... def inverse(self, value): """ Maps the normalized value (i.e., index in the colormap) back to image data value. Parameters ---------- value Normalized value. """ ... def autoscale(self, A): """Set *vmin*, *vmax* to min, max of *A*.""" ... def autoscale_None(self, A): """If *vmin* or *vmax* are not set, use the min/max of *A* to set them.""" ... def scaled(self): """Return whether *vmin* and *vmax* are both set.""" ...and some of the docstrings will need to be updated to also allow for MultiNorm, but this can happen in the next PR.
This requires a lot more than the bare minimum. I tested, and I can do a plt.imshow() with just __call__ and autoscale_None, but I get the feeling that it is desirable to lock it down more.