Movatterモバイル変換


[0]ホーム

URL:


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

Abstract base class for Normalize#30178

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Open
trygvrad wants to merge3 commits intomatplotlib:main
base:main
Choose a base branch
Loading
fromtrygvrad:norm_ABC

Conversation

trygvrad
Copy link
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)

@tacaswell
Copy link
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 cleartechnical 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.

@tacaswell
Copy link
Member

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

Copy link
Member

@tacaswelltacaswell left a 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.

@QuLogic
Copy link
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

@timhoffm
Copy link
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

@github-actionsgithub-actionsbot added the Documentation: APIfiles in lib/ and doc/api labelJun 24, 2025
@trygvrad
Copy link
ContributorAuthor

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

@trygvrad
Copy link
ContributorAuthor

I want to point out that at the end ofcolors.py there is a private function_make_norm_from_scale() that defines a classNorm().
Do we care that we now have two classes namedNorm in the context of this helper function, or should we rename this toNewNorm orNormFromScale etc.?

@functools.cachedef_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.    """classNorm(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.

@QuLogic
Copy link
Member

It doesn't create a class namedNorm, 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

@timhoffm
Copy link
Member

Let's rename it toScaleNorm orNormTemplate.

trygvrad reacted with thumbs up emoji

@trygvrad
Copy link
ContributorAuthor

trygvrad commentedJun 25, 2025
edited
Loading

I changed it toScaleNorm

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


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

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

Copy link
Member

@timhoffmtimhoffm left a comment
edited
Loading

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.

@story645
Copy link
Member

story645 commentedJun 26, 2025
edited
Loading

Are all norms invertible?

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

@timhoffm
Copy link
Member

timhoffm commentedJun 27, 2025
edited
Loading

Actually, all norms are not invertible until they are scaled

ifnotself.scaled():
raiseValueError("Not invertible until both vmin and vmax are set")

(or if vmin=vmax).

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

ifnp.isfinite(normed):
ifisinstance(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()
elifself.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 getdelta / the number of significant digits.

We could make the API forinverse() more explicit; i.e. make invertable optional and define errors if they are not. But sinceinverse() is only used in the above context, it may be better to not adoptinverse() in the baseNorm 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.

Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@QuLogicQuLogicQuLogic left review comments

@tacaswelltacaswelltacaswell approved these changes

@timhoffmtimhoffmtimhoffm approved these changes

Assignees
No one assigned
Labels
Projects
None yet
Milestone
v3.11.0
Development

Successfully merging this pull request may close these issues.

5 participants
@trygvrad@tacaswell@QuLogic@timhoffm@story645

[8]ページ先頭

©2009-2025 Movatter.jp