-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
Proposal: Implement RcParams using ChainMap and remove dict inheritance
#25617
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
RcParams using ChainMap and remove dict inheritance
|
Interesting. I see the benefit for doing the context (and it might be a better way to keep the default defaults aronud!) However I'm not sure how the |
|
Yeah. It started as updating the ChainMap getters/setters but I was checking the code and the final Though, I'm not entirely sure how to implement |
ff13a4c to
8f3c3b2
Compare
RcParams using ChainMap and remove dict inheritanceRcParams using ChainMap and remove dict inheritance
7ebe855 to
88b068d
Compare
|
@ksunden had to change a few things in the stubs file too but not very sure of the changes. Would appreciate it if you can take a look. Thanks! |
| namespaces: tuple | ||
| single_key_set: set |
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.
Can these be more specific about the type of the values?
e.g. tuple[str, ...] (this is how you say homogeneous tuple of str, indeterminate length)
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.
Ah, thanks! I wasn't sure how to explain indeterminate length so kept it as just a tuple. I'll update it.
ksunden
left a comment
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.
While my more concrete comments are about the type hinting side of things, I'm not super sold on changing the canonical names we use for the keys in the "default" namespace, even with some level of backwards compatibility...
| def _get(self, key: str) -> Any: ... | ||
| def __setitem__(self, key: str, val: Any) -> None: ... | ||
| def __getitem__(self, key: str) -> Any: ... | ||
| def __delitem__(self, key: str) -> None: ... |
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.
This is actually inherited from MutableMapping, but doesn't hurt to put it here more explicitly...
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.
I added these mainly because we overwrite these functions. So, just to be consistent with having stubs for the functions implemented..
I'm not very fond of the name Or do you mean changing the names in |
|
Yeah, mainly I'm thinking that any and all usage should likely stay with the unnamespaced name (in rc files, but also in code) and whatever designation is used should be purely internal to the class. |
d678c93 to
7361150
Compare
7361150 to
dcfaf01
Compare
lib/matplotlib/__init__.py
Outdated
| elif depth == 2: | ||
| return self._namespace_maps[keys[0]].maps[-1].get(keys[1]) | ||
|
|
||
| def getdefault(self, key): |
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.
We should follow the naming convention here. While some stdlib API does not use underscores for historic reasons (e.g. dict.setdefault) we should follow the naming convention and not some special historic cases.
| def getdefault(self, key): | |
| def get_default(self, key): |
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.
I don't mind changing this but 1. Do we change setdefault to set_default too? and as you mentioned 2. Do we still keep setdefault?
lib/matplotlib/__init__.py
Outdated
|
|
||
| return self._get_default(key) | ||
|
|
||
| def getdefaults(self): |
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.
| def getdefaults(self): | |
| def get_defaults(self): |
lib/matplotlib/__init__.py
Outdated
| raise | ||
|
|
||
| config = RcParams() | ||
| config._parents() |
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.
Please explain what this does and why we need it (at least in its docstring). The need to call a private method feels like a design flaw to me.
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.
Okay so this is a little ugly and I'm not very convinced either but the idea is - Since we're initializing RcParams with an empty dictionary before populating it with the default values, this leads to RcParams being initialized as basically ChainMap({}, {}). So, all the updates are then done in the first dictionary and the last dictionary just remains as an empty dictionary. So, to have the defaults in the last layer and have something like ChainMap({}, defaults) instead of ChainMap({}, defaults, {}), I remove the added layer and once all defaults have been set, I add a new layer.
Ideally, I would want to initialize it as RcParams(defaults) and not RcParams() and update then.
lib/matplotlib/__init__.py
Outdated
| or from the matplotlib source distribution""", | ||
| dict(key=key, fname=fname, line_no=line_no, | ||
| line=line.rstrip('\n'), version=version)) | ||
| config._new_child() |
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.
As above: Please explain what this does and why we need it (at least in its docstring). The need to call a private method feels like a design flaw to me.
|
I'm not convinced by the proposed namespacing concept: It seems to hard-code one level space level, e.g. for To keep things simple (at least for a start), I suggest not to mix namespacing and chainmap. Let's keep the full dotted names as keys in the internal structure and use a simple two-element ChainMap (untested code): This should have fairly straight forward implementations for the MutableMapping interface. You can still build grouping in later dynamically (untested code): That's a bit more expensive but likey bearable. (One could also cache the prefix -> full keys mappings.) |
|
Thanks, @timhoffm. Just a small note, the one level namespace is not working because I intentionally removed it in the last commit based on the discussion in a dev call. In any case, I see your point. Let me go back and think about this again. I plan to get back to this again sometime this week. |
af93c0b to
56fc4d6
Compare
|
@timhoffm, I narrowed the scope as you suggested and just focused on removing the dictionary inheritance and the |
| def setdefault(self, key, default=None): | ||
| """Insert key with a value of default if key is not in the dictionary. | ||
| Return the value for key if key is in the dictionary, else default. | ||
| """ | ||
| if key in self: | ||
| return self[key] | ||
| self[key] = default | ||
| return default |
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.
Current setdefault behavior (side-effect of __setitem__ not accepting new keys): Return the value if key is valid, otherwise raise. We should keep this behavior for compatibility.
Note that this behavoir is identical to rcParams[key]. Optional: Deprecate setdefault and recommend rcParams[key] instead.
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.
I think we should just return the value (maybe catching the KeyError on misses to match the current error message exactly) and explicitly handle the one case we use this internally .
1b98a07 to
b5daa3e
Compare
This is to make the interface such that the code other than RcParams doesn't have to deal with the `deafult.*` namespace anywhere. This also changes the keys returned by `.keys()` to not have the `default.*` in the key name.
As per @timhoffm's suggestion, decreasing scope of this PR to first just remove the dict inheritance and add a ChainMap to maintain settings. This is a fairly straightforward change and doesn't change the interface. Furthermore, the keys are still dotted and don't support namespacing as defining namespaces might take a little more discussion.
ab37ed5 to
dfbb73f
Compare
PR Summary
The current implementation of
RcParamsinherits bothMutableMappinganddict. Inheriting fromdicthas a few problems IMO,dictget propagated toRcParamsdirectly. (Can't import matplotlib #12601)dictmeansRcParamscan be modified usingdict.__*__methods. As a configuration class for matplotlib, I think all updates toRcParamsshould be possible only using methods defined by the class. (Data access API for rcParams #24730, RcParams should not inherit from dict #12577 )RcParamswhich would affect the working of the library.RcParamsas a store for keys other than the configuration keys with validators.Proposed Implementation:
dictinheritance and just inheritMutableMappingto main that interface.For example:
Where
default_<namespace>_valuesare the default values of theRcParamsunder that namespace. For example.Advantages:
RcParamsDefaultobject as these defaults will be present in the base default values of the ChainMaps.new_childandparentsin ChainMaps as compared to the the current method of creating and deletingRcParamsobjects.would output
Disadvantages:
popitem()method forMutableMappingis slightly unruly to implement. (Though, I would argue that it should not work onRcParamsin any case). Also, relevant discussion here (RcParams should not inherit from dict #12577 (comment))Doing this will also allow us to move some extra functions that exist in the
matplotlibnamespace to theRcParamsclass instead, clearing up the namespace a little bit. (#2052)Performance Impacts
Introducing this structure instead of the entire
rcParamsbeing a dictionary makes it a little slower. The timings for a get operation on both the methods were measured using%timeitfor 1,000,000 loops each.Current Implementation (dict):
Proposed Implementation (ChainMaps):
For completion, while disabled right now, the timing for
mpl.rcParams["animation"]["convert_args"]is also measuredOlder description
I removed the
dictinheritance from theRcParamsclass and reimplemented its internals using ChainMaps. The interface is still compatible withMutableMappinginterface. Using ChainMaps allows namespace access too, for examplercParams['text']will return all the properties undertext.*. I also like the way it allows context management usingnew_child()andparents. The main goal of the PR is just to see what a different implementation might look like per #24585 and have some more discussion.This implementation can be made efficient. I also have changed a few things like
backendtodefault.backendwhich is something we might not want in the actual implementation but I have changed here because the focus was on having an implementation and not worrying way too much about changes. It's mainly addingdefault.to the settings that did not fall under any "category".I did not implement the
find_allmethod initially because from a quick github search, it isn't used anywhere, but it can now be implemented fairly easily by searching on the keys. The__repr__and__str__differ from the current implementation as they have a very simple implementation right now.PR Checklist
Documentation and Tests
pytestpasses)Release Notes
.. versionadded::directive in the docstring and documented indoc/users/next_whats_new/.. versionchanged::directive in the docstring and documented indoc/api/next_api_changes/next_whats_new/README.rstornext_api_changes/README.rst