| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
|
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. |
Sorry, something went wrong.
|
The docs failure is real, The new class needs to be manually added to the rst. |
Sorry, something went wrong.
There was a problem hiding this comment.
modulo fixing the docs build.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
The doc build failure looks a bit cryptic, but I think putting Norm here will solve it |
Sorry, something went wrong.
|
Thank you @tacaswell @timhoffm @QuLogic |
Sorry, something went wrong.
|
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(). @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. |
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
Let's rename it to ScaleNorm or NormTemplate. |
Sorry, something went wrong.
|
I changed it to ScaleNorm I think this PR is ready to be merged now :) EDIT: I spoke before the tests had completed |
Sorry, something went wrong.
|
|
||
| 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__.
Sorry, something went wrong.
There was a problem hiding this comment.
The code is ok. Two remarks
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.
BoundaryNorm isn't (or is only invertible up to bin edges). |
Sorry, something went wrong.
|
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 inverse() and all the non-invertable cases are explicitly checked before: matplotlib/lib/matplotlib/colorizer.py Lines 483 to 502 in c78c2f4 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. |
Sorry, something went wrong.
|
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 |
Sorry, something went wrong.
|
Merging as is. We can improve on the inverse handling separately. |
Sorry, something went wrong.
| Back | FazBrowse Home | New Git URL |
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)