Abstract base class for Normalize#30178
Conversation
|
After discussion on this weeks call I think we should go with the ABC flavor. Given:
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. |
|
The docs failure is real, The new class needs to be manually added to the rst. |
tacaswell
left a comment
There was a problem hiding this comment.
modulo fixing the docs build.
|
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 |
|
The doc build failure looks a bit cryptic, but I think putting Norm here will solve it |
|
Thank you @tacaswell @timhoffm @QuLogic |
|
I want to point out that at the end of @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. |
|
It doesn't create a class named |
|
Let's rename it to |
|
I changed it to I think this PR is ready to be merged now :) EDIT: I spoke before the tests had completed |
lib/matplotlib/colors.py
Outdated
|
|
||
| def __init__(self): | ||
| """ | ||
| Abstract base class for normalizations. |
There was a problem hiding this comment.
Docstring seems like it should be on the class, not the __init__.
There was a problem hiding this comment.
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.
BoundaryNorm isn't (or is only invertible up to bin edges). |
|
Actually, all norms are not invertible until they are scaled matplotlib/lib/matplotlib/colors.py Lines 2459 to 2460 in c78c2f4 (or if vmin=vmax). There's only one usage of matplotlib/lib/matplotlib/colorizer.py Lines 483 to 502 in c78c2f4 In fact, the whole song and dance is only done to get We could make the API for |
|
I removed I think it would be good to merge this now, so I can go back to working on #29876 |
|
Merging as is. We can improve on the inverse handling separately. |
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)