-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
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.
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 __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
@@ -890,6 +1057,7 @@ def _rc_params_in_file(fname, transform=lambda x: x, fail_on_error=False): | |||
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
@@ -918,6 +1086,7 @@ def _rc_params_in_file(fname, transform=lambda x: x, fail_on_error=False): | |||
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
RcParams
inherits bothMutableMapping
anddict
. Inheriting fromdict
has a few problems IMO,dict
get propagated toRcParams
directly. (Can't import matplotlib #12601)dict
meansRcParams
can be modified usingdict.__*__
methods. As a configuration class for matplotlib, I think all updates toRcParams
should be possible only using methods defined by the class. (Data access API for rcParams #24730, RcParams should not inherit from dict #12577 )RcParams
which would affect the working of the library.RcParams
as a store for keys other than the configuration keys with validators.Proposed Implementation:
dict
inheritance and just inheritMutableMapping
to main that interface.For example:
Where
default_<namespace>_values
are the default values of theRcParams
under that namespace. For example.Advantages:
RcParamsDefault
object as these defaults will be present in the base default values of the ChainMaps.new_child
andparents
in ChainMaps as compared to the the current method of creating and deletingRcParams
objects.would output
Disadvantages:
popitem()
method forMutableMapping
is slightly unruly to implement. (Though, I would argue that it should not work onRcParams
in 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
matplotlib
namespace to theRcParams
class instead, clearing up the namespace a little bit. (#2052)Performance Impacts
Introducing this structure instead of the entire
rcParams
being a dictionary makes it a little slower. The timings for a get operation on both the methods were measured using%timeit
for 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
dict
inheritance from theRcParams
class and reimplemented its internals using ChainMaps. The interface is still compatible withMutableMapping
interface. 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
backend
todefault.backend
which 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_all
method 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
pytest
passes)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.rst
ornext_api_changes/README.rst