GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Abstract base class for Normalize by trygvrad · Pull Request #30178 · matplotlib/matplotlib

Conversation

@trygvrad

@trygvrad trygvrad commented Jun 15, 2025

Copy link
Copy Markdown
Contributor

This PR is an alternative to #30149
It is created so that we can contrast full implementations of a protocol vs an abstract base class in light of the comment here #30149 (comment)
For context on why this is required, see the comment on the MultiNorm PR #29876 (comment)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@trygvrad
Abstract base class for Normalize
fc94ae1
@github-actions github-actions Bot added the topic: color/color & colormaps label Jun 15, 2025
@trygvrad trygvrad mentioned this pull request Jun 15, 2025
Creation of the Norm Protocol #30149
Closed
@tacaswell tacaswell added this to the v3.11.0 milestone Jun 19, 2025
@tacaswell

tacaswell commented Jun 19, 2025

Copy link
Copy Markdown
Member

After discussion on this weeks call I think we should go with the ABC flavor. Given:

  • from a technical point of view both do the job and there is no clear technical winner so it is subjective call
  • @timhoffm prefers ABCs
  • some of our core developers learned Protocols exist due to this discussion
  • ABCs have been around a lot longer than Protocols
  • we don't have any Protocols currently in the code base

If we are going to bring in a new language feature we should do that when it brings in a clear benefit so we should stick with ABC.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@tacaswell

tacaswell commented Jun 19, 2025

Copy link
Copy Markdown
Member

The docs failure is real, The new class needs to be manually added to the rst.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

tacaswell
tacaswell approved these changes Jun 19, 2025
View reviewed changes

@tacaswell tacaswell left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

modulo fixing the docs build.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions
@QuLogic

QuLogic commented Jun 19, 2025

Copy link
Copy Markdown
Member

If I'm not mistaken, once you put the base class into Sphinx, you can replace the duplicate docstrings in the no-longer-base class with # docstring inherited.

timhoffm reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@timhoffm

timhoffm commented Jun 20, 2025

Copy link
Copy Markdown
Member

The doc build failure looks a bit cryptic, but I think putting Norm here will solve it
https://github.com/matplotlib/matplotlib/blob/main/doc/api/colors_api.rst#color-norms

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@github-actions github-actions Bot added the Documentation: API files in lib/ and doc/api label Jun 24, 2025
@trygvrad
trygvrad force-pushed the norm_ABC branch from f762fa6 to d26581f Compare June 24, 2025 21:08
@trygvrad
Include the Norm ABC in the docs
5e43a75
@trygvrad
trygvrad force-pushed the norm_ABC branch from d26581f to 5e43a75 Compare June 24, 2025 21:45
@trygvrad

trygvrad commented Jun 24, 2025

Copy link
Copy Markdown
Contributor Author

Thank you @tacaswell @timhoffm @QuLogic
The docs should be fixed now, and I replaced the docstrings of the methods in Normalize with # docstring inherited

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@trygvrad

trygvrad commented Jun 24, 2025

Copy link
Copy Markdown
Contributor Author

I want to point out that at the end of colors.py there is a private function _make_norm_from_scale() that defines a class Norm().
Do we care that we now have two classes named Norm in the context of this helper function, or should we rename this to NewNorm or NormFromScale etc.?

@functools.cache
def _make_norm_from_scale(
    scale_cls, scale_args, scale_kwargs_items,
    base_norm_cls, bound_init_signature,
):
    """
    Helper for `make_norm_from_scale`.

    This function is split out to enable caching (in particular so that
    different unpickles reuse the same class).  In order to do so,

    - ``functools.partial`` *scale_cls* is expanded into ``func, args, kwargs``
      to allow memoizing returned norms (partial instances always compare
      unequal, but we can check identity based on ``func, args, kwargs``;
    - *init* is replaced by *init_signature*, as signatures are picklable,
      unlike to arbitrary lambdas.
    """

    class Norm(base_norm_cls):
        def __reduce__(self):
            cls = type(self)
            # If the class is toplevel-accessible, it is possible to directly
            # pickle it "by name".  This is required to support norm classes
            # defined at a module's toplevel, as the inner base_norm_cls is
            # otherwise unpicklable (as it gets shadowed by the generated norm
            # class).  If either import or attribute access fails, fall back to
            # the general path.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@QuLogic

QuLogic commented Jun 24, 2025

Copy link
Copy Markdown
Member

It doesn't create a class named Norm, but <Scale>Norm. I suppose we should rename the temporary to reduce confusion, but it's not required from a technical point-of-view.

trygvrad reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@timhoffm

timhoffm commented Jun 25, 2025

Copy link
Copy Markdown
Member

Let's rename it to ScaleNorm or NormTemplate.

trygvrad reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@trygvrad
Changed name of temporary class Norm to ScaleNorm in _make_norm_from_…
a146d99
…scale()
@trygvrad

trygvrad commented Jun 25, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor Author

I changed it to ScaleNorm

I think this PR is ready to be merged now :)

EDIT: I spoke before the tests had completed
EDIT 2: I am not able to relate the test that fails on macos-13/python 3.11 to any changes in this PR.
FAILED lib/matplotlib/tests/test_backends_interactive.py::test_blitting_events[MPLBACKEND=macosx-BACKEND_DEPS=matplotlib.backends._macosx] - assert 0 < 0

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

QuLogic
QuLogic reviewed Jun 26, 2025
View reviewed changes
Comment thread lib/matplotlib/colors.py Outdated

def __init__(self):
"""
Abstract base class for normalizations.

@QuLogic QuLogic Jun 26, 2025

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Docstring seems like it should be on the class, not the __init__.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

trygvrad reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
timhoffm
timhoffm approved these changes Jun 26, 2025
View reviewed changes

@timhoffm timhoffm left a comment •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The code is ok. Two remarks

  • I’m not quite sure whether inverse() should be part of the base class. - Are all norms invertible?
  • Documentation could be improved, but I don’t have the time to comment in more detail right now. Will do later.

Both aspects should be looked at before public release, but I don’t want to hold up further work for that. You may merge now if it enables further work.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

trygvrad reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@story645

story645 commented Jun 26, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Are all norms invertible?

BoundaryNorm isn't (or is only invertible up to bin edges).

trygvrad reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@timhoffm

timhoffm commented Jun 27, 2025 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Actually, all norms are not invertible until they are scaled

matplotlib/lib/matplotlib/colors.py

Lines 2459 to 2460 in c78c2f4

if not self.scaled():
raise ValueError("Not invertible until both vmin and vmax are set")

(or if vmin=vmax).

There's only one usage of inverse() and all the non-invertable cases are explicitly checked before:

matplotlib/lib/matplotlib/colorizer.py

Lines 483 to 502 in c78c2f4

if np.isfinite(normed):
if isinstance(self.norm, colors.BoundaryNorm):
# not an invertible normalization mapping
cur_idx = np.argmin(np.abs(self.norm.boundaries - data))
neigh_idx = max(0, cur_idx - 1)
# use max diff to prevent delta == 0
delta = np.diff(
self.norm.boundaries[neigh_idx:cur_idx + 2]
).max()
elif self.norm.vmin == self.norm.vmax:
# singular norms, use delta of 10% of only value
delta = np.abs(self.norm.vmin * .1)
else:
# Midpoints of neighboring color intervals.
neighbors = self.norm.inverse(
(int(normed * n) + np.array([0, 1])) / n)
delta = abs(neighbors - data).max()
g_sig_digits = cbook._g_sig_digits(data, delta)
else:
g_sig_digits = 3 # Consistent with default below.

In fact, the whole song and dance is only done to get delta / the number of significant digits.

We could make the API for inverse() more explicit; i.e. make invertable optional and define errors if they are not. But since inverse() is only used in the above context, it may be better to not adopt inverse() in the base Norm at all. Instead, let the norms provide the relevant information (delta / the number of significant digits). This also removes all the special checking in the code block above.

trygvrad reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@trygvrad
removal of inverse() in Norm ABC
81fa790
@trygvrad

trygvrad commented Jun 28, 2025

Copy link
Copy Markdown
Contributor Author

I removed inverse() from Norm and moved the docstring from init to the class :)

I think it would be good to merge this now, so I can go back to working on #29876

timhoffm reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

timhoffm
timhoffm approved these changes Jun 28, 2025
View reviewed changes
@timhoffm
timhoffm merged commit 7edd74a into matplotlib:main Jun 28, 2025
@timhoffm

timhoffm commented Jun 28, 2025

Copy link
Copy Markdown
Member

Merging as is. We can improve on the inverse handling separately.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

@trygvrad trygvrad mentioned this pull request Jun 29, 2025
MultiNorm class #29876
Merged
@trygvrad trygvrad moved this to Superseded in Bivariate and Multivariate Colormapping Jul 3, 2025
@trygvrad trygvrad added this to Bivariate and Multivariate Colormapping Jul 3, 2025
@trygvrad trygvrad moved this from Superseded to Done in Bivariate and Multivariate Colormapping Jul 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@QuLogic QuLogic QuLogic left review comments
@tacaswell tacaswell tacaswell approved these changes
@timhoffm timhoffm timhoffm approved these changes

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

Documentation: API files in lib/ and doc/api topic: color/color & colormaps

Projects

Bivariate and Multivariate Colormapping
Status: Done

Milestone

v3.11.0

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

5 participants

@trygvrad @tacaswell @QuLogic @timhoffm @story645
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402520 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.