Speedup normalize_kwargs by storing aliases in a more practical format.#31023
Speedup normalize_kwargs by storing aliases in a more practical format.#31023timhoffm merged 1 commit intomatplotlib:mainfrom
Conversation
anntzer
force-pushed
the
nk
branch
3 times, most recently
from
aba7544 to
f09995e
Compare
lib/matplotlib/tests/test_cbook.py
Outdated
| ( | ||
| {"a": 1, "b": 2}, | ||
| _api.define_aliases({"a": ["b"]})( | ||
| type("Type", (mpl.artist.Artist,), {"get_a": lambda self: None})), |
There was a problem hiding this comment.
Since the undecorated type is always the same, is it worth defining it inside the tests and only parametrizing the dictionary? Or defining it once before fail_mapping and pass_mapping?
There was a problem hiding this comment.
The decorator modifies the underlying class in-place (it does not return a new class), so you need to generate a new class each time (because the aliases are not always the same).
There was a problem hiding this comment.
Oh yes of course, so you cannot define it before. But I still think it could be defined within the tests.
lib/matplotlib/_api/__init__.py
Outdated
| raise NotImplementedError( | ||
| f"Parent class already defines conflicting aliases: {conflicting}") | ||
| cls._alias_map = {**preexisting_aliases, **alias_d} | ||
| cls._alias_map = {**preexisting_aliases, **alias_to_prop} |
There was a problem hiding this comment.
For clarity of the mapping, and to make the semantic change more obvious, can we rename this
| cls._alias_map = {**preexisting_aliases, **alias_to_prop} | |
| cls._alias_to_prop = {**preexisting_aliases, **alias_to_prop} |
lib/matplotlib/cbook.pyi
Outdated
| def normalize_kwargs( | ||
| kw: dict[str, Any], | ||
| alias_mapping: dict[str, list[str]] | type[Artist] | Artist | None = ..., | ||
| alias_mapping: type[Artist] | Artist = ..., |
There was a problem hiding this comment.
| alias_mapping: type[Artist] | Artist = ..., | |
| alias_mapping: type[Artist] | Artist | None = ..., |
I haven't checked but I think this will clear up the stubtest failure. We do not have guidance for removal of a default in the guide, but I think the closest is
Items decorated with
@_api.delete_parametershould include a default value hint for the deleted parameter
https://matplotlib.org/devdocs/devel/api_changes.html#introduce-deprecation
There was a problem hiding this comment.
thanks for looking into it, hopefully fixed now.
Directly store property alias information in `{"alias": "propety", ...}`
format, which greatly speeds up normalize_kwargs.
In order to not worry about two different dict formats (the new one, and
the old one, which was `{"property": ["alias", ...], ...}`) in
normalize_kwargs, deprecate support for directly passing an alias map --
callers should always pass an artist (or artist type) directly, which
hides the detail of the format of the alias map. Also deprecate passing
`alias_mapping=None`, which doesn't really seem needed (unlike
`kw=None`, which is regularly used).
Whether the format of aliases passed to `_api.define_aliases` should
also be changed to match the new format is a separate question that
doesn't have to be addressed here (it can be changed later, being fully
private anyways).
4 participants
Directly store property alias information in
{"alias": "propety", ...}format, which greatly speeds up normalize_kwargs.In order to not worry about two different dict formats (the new one, and the old one, which was
{"property": ["alias", ...], ...}) in normalize_kwargs, deprecate support for directly passing an alias map -- callers should always pass an artist (or artist type) directly, which hides the detail of the format of the alias map. Also deprecate passingalias_mapping=None, which doesn't really seem needed (unlikekw=None, which is regularly used).Whether the format of aliases passed to
_api.define_aliasesshould also be changed to match the new format is a separate question that doesn't have to be addressed here (it can be changed later, being fully private anyways).See discussion at #30979, which this partially addresses.
PR summary
PR checklist