Skip to content

Data access API for rcParams #24730

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Dec 23, 2022
Merged

Data access API for rcParams #24730

merged 1 commit into from
Dec 23, 2022

Conversation

timhoffm
Copy link
Member

@timhoffm timhoffm commented Dec 14, 2022 •
edited
Loading

PR Summary

Replaces (first step of) #12577, see in particular #12577 (comment).

This provides a defined API for accessing rcParams via rcParams._data[key] while circumventing any validation logic happening in rcParams[key].

Before, direct data access was realized through dict.__getitem__(rcParams, key) / dict.__setitem__(rcParams, key, val), which depends on the implementation detail of rcParams being a dict subclass. The new data access API gets rid of this dependency and thus opens up a way to later move away from dict subclassing.

We want to move away from dict subclassing and only guarantee the MutableMapping interface for rcParams in the future. This allows other future restructings like introducing a new configuration management and changing rcParams into a backward-compatible adapter.

Ping @anntzer who will be affected by this change in mplcairo #12577 (comment)

@timhoffm timhoffm added this to the v3.7.0 milestone Dec 14, 2022
@anntzer
Copy link
Contributor

anntzer commented Dec 14, 2022 •
edited
Loading

Thanks for the ping. The replacement seems OK, but perhaps you could just make the API rcParams._get(key) and rcParams._set(key, value) which avoids introducing another private helper class and can just be done with plain methods? (Also you could mark the methods with :meta public: so that they show up in the rendered docs, see https://www.sphinx-doc.org/en/master/usage/extensions/autodoc.html)

@timhoffm timhoffm force-pushed the rcparams-data-api branch 2 times, most recently from e8ff5bb to 888811c Compare December 15, 2022 10:37
oscargus
oscargus reviewed Dec 15, 2022
View reviewed changes
Copy link
Member

@oscargus oscargus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some minor comments. Looks good, but I guess other people have better insights into what is actually wanted.

lib/matplotlib/__init__.py
@@ -605,7 +605,7 @@ def gen_candidates():
)
class RcParams(MutableMapping, dict):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I may not fully get the scope here, but I take it that one reason to do this is that we should not subclass from dict later on? Or is the whole idea to provide a stable API for this, but actually still subclass?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to get rid of dict subclassing eventually. Using a dict subclass dictates the internal data model. We likely don't want this in the future when we're remodelling the config data structure. Either the new structure will be a 100% API compatible drop in and the config object will be available via rcParams; or we have a new completely free to design config object and rcParams will loose all state and only be an adapter to that new config object. Either way, being bound to a dict subclass would be cumbersome.

@jklymak
Copy link
Member

jklymak commented Dec 15, 2022

I guess I don't fully understand the need for this change, even after reading the comments before. Is there some reason _set and _get wouldn't just be aliased to __setitem__ and __getitem__ in the new class, even without subclassing dict?

@jklymak
Copy link
Member

jklymak commented Dec 15, 2022

Note CI didn't run for this.

@tacaswell
Copy link
Member

Discussed on call, consensus is that while _get and _set are good ideas, we should refrain from making any major changes to the current implementation until we have a better idea what we want to replace it with.

jklymak
jklymak reviewed Dec 15, 2022
View reviewed changes
Copy link
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I didn't grok that this was calling dict.__get_item__, and hence didn't understand the point of this. I now understand and think it is a reasonable thing to add direct private access without hacking up an inheritance level to dict. Added a few more clarifying phrases to the note that would have helped me.

story645
story645 reviewed Dec 22, 2022
View reviewed changes
Copy link
Member

@story645 story645 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have no objection on adding a shim/translation layer, especially if it's kept private 'til an API gets settled on - mostly just documentation nits about how this is communicating that this is a "don't touch, for contributors only" API.

@timhoffm @oscargus
@jklymak @QuLogic @story645
Data access API for rcParams
ecf156c 
This provides a defined API for accessing rcParams via
`rcParams._data[key]`
while circumventing any validation logic happening in
`rcParams[key]`.

Before, direct data access was realized through
`dict.__getitem(rcParams, key)` / `dict.__setitem(rcParams, key, val)`,
which depends on the implementation detail of `rcParams` being a dict
subclass. The new data access API gets rid of this dependence and thus
opens up a way to later move away from dict subclassing.

We want to move away from dict subclassing and only guarantee the
`MutableMapping` interface for `rcParams` in the future. This allows
other future restructings like introducing a new configuration management
and changing `rcParams` into a backward-compatible adapter.

Co-authored-by: Oscar Gustafsson <oscar.gustafsson@gmail.com>
Co-authored-by: Jody Klymak <jklymak@gmail.com>
Co-authored-by: Elliott Sales de Andrade <quantum.analyst@gmail.com>
Co-authored-by: story645 <story645@gmail.com>
@ksunden ksunden merged commit 488962a into matplotlib:main Dec 23, 2022
@timhoffm timhoffm deleted the rcparams-data-api branch December 24, 2022 00:32
@chahak13 chahak13 mentioned this pull request Apr 13, 2023
Draft
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@QuLogic QuLogic QuLogic left review comments

@jklymak jklymak jklymak left review comments

@oscargus oscargus oscargus left review comments

@tacaswell tacaswell tacaswell approved these changes

@story645 story645 story645 approved these changes

Assignees
No one assigned
Labels
API: changes Maintenance
Projects
None yet
Milestone
v3.7.0
Development

Successfully merging this pull request may close these issues.
8 participants
@timhoffm @anntzer @jklymak @tacaswell @QuLogic @story645 @oscargus @ksunden