Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

Closed
dstansby wants to merge 1 commit into matplotlib:master from dstansby:norm-base
Closed

Conversation

Copy link
Member

@dstansby dstansby commented Jan 22, 2020
edited
Loading

A draft of creating a base class for Norms. I've called it simply Norm, on the basis that the base for colorbars is simply Colorbar.

With these changes:

  • There is an abstract base class called Norm, which all norms should inherit from
  • The linear normaliser (previously Normalize) is now called LinearNorm
  • Normalize still works as before, with a deprecation warning

Reasons this is useful

  • Currently all norms inherit from Normalize, so if they don't define __call__ or inverse they silently inherit linear verisons, which could be confusing
  • LinearNorm is a much clearer name than Normalize for a linear normalisation.
  • Adding 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__ and inverse to be defined for all norms.

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 needs a comprehensive changelog.

Copy link
Member

jklymak commented Jan 22, 2020

Can you say more about why this is useful?

Copy link
Contributor

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.

Copy link
Member Author

Can you say more about why this is useful?

Added to the description.

Copy link
Member

jklymak commented Jan 22, 2020

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.

Copy link
Member Author

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.

Copy link
Member

jklymak commented Jan 22, 2020

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?

Copy link
Contributor

anntzer commented Jan 22, 2020
edited
Loading

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

Copy link
Member

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.

timhoffm and dstansby reacted with thumbs up emoji

Copy link
Member

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.

@QuLogic QuLogic modified the milestones: v3.3.0, v3.4.0 May 5, 2020
@QuLogic QuLogic modified the milestones: v3.4.0, v3.5.0 Jan 21, 2021
Copy link
Member Author

dstansby commented May 3, 2021

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.

@dstansby dstansby deleted the norm-base branch December 31, 2023 10:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Projects
None yet
Milestone
v3.5.0
Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /