-
-
Notifications
You must be signed in to change notification settings - Fork 8k
Split Norm and LinearNorm up #16291
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
Split Norm and LinearNorm up #16291
Conversation
Can you say more about why this is useful?
How would you feel about Normalizer
as the name of the base class that all others could inherit from? Then you could do isinstance(norm, Normalizer)
, which sounds better and more explicit to me.
Can you say more about why this is useful?
Added to the description.
Thanks! I'm not 100% convinced this is worth the breakage. I can imagine isinstance(Normalize) gets called somewhat often? Are we worried about code-base norms or user-defined norms being broken? This is a mild concern on my part, so not at all blocking if other folks think this is helpful.
I do think there is overall call to re-engineer norms and scales in a back-compat breaking "Matplotlib 4.0". Though I'm a little confused where we are keeping track of those issues.
Now I come to think of it, the other way of doing it would be to leave Normalize
as is, but deprecate Normalize.__call__
and Normalize.inverse
, in favour of new LinearNorm
methods, which would keep isintance(Normalize)
working fine.
But you'd have to deprecate __init__
as well? There have to be a lot of folks who are calling norm=Normalize(vmin, vmax)
perhaps in the context of switching norms back and forth?
n.b. this will break isinstance(norm, Normalize) - instead users will need to do isinstance(norm, Norm). Are we happy making this breaking change? I couldn't think of a way to deprecate that kind of thing.
This is actually not too hard to handle (ie emit a proper deprecation) by using __subclasscheck__
and __instancecheck__
(will need a tiny metaclass, but nothing too horrible).
There seems to be two independent problems here:
Problem 1: Norms inherit from Normalize
instead of a more general base class.
Problem 2: The name Normalize
is slightly off or misleading.
I would propose to treat them separately; and for Problem 2 I would propose to think very very hard about if the name is really bad enough to merit annoying basically everyone, including numerous downstream packages, with a deprecation.
Norms are likely used in 3rd party libs. We should give them a head start on this change, otherwise we force them to release before the next matplotlib release (or their users will get the depreactions without being responsible. IMHO, we should either
- soft-deprecate, similar to what we do with pylab, or
- use a pending deprecation.
I'm not yet clear myself, which base class name I'd prefer.
Since I don't think I have any bandwidth in the near future to finish this off, I'm going to close it, but I'll leave my branch around in case anyone wants to pick it up.
Uh oh!
There was an error while loading. Please reload this page.
A draft of creating a base class for Norms. I've called it simply
Norm
, on the basis that the base for colorbars is simplyColorbar
.With these changes:
Norm
, which all norms should inherit fromNormalize
) is now calledLinearNorm
Normalize
still works as before, with a deprecation warningReasons this is useful
Normalize
, so if they don't define__call__
orinverse
they silently inherit linear verisons, which could be confusingLinearNorm
is a much clearer name thanNormalize
for a linear normalisation.Norm
as an abstract base class with non-specific implementations allows for generic documentation on how to construct norm classes, and making it an abstract class forces__call__
andinverse
to be defined for all norms.n.b. this will break
isinstance(norm, Normalize)
- instead users will need to doisinstance(norm, Norm)
. Are we happy making this breaking change? I couldn't think of a way to deprecate that kind of thing.This needs a comprehensive changelog.