RcParams should not inherit from dict #12577

timhoffm · 2018-10-20T21:42:58Z

PR Summary

timhoffm · 2018-10-20T21:46:40Z

For now, I've replaced the dict.__set/getitem__(matplotlib.rcParams, ...) calls with private data access matplotlib.rcParams._data[...]. Considering to establish a public way to bypass the checks.

timhoffm · 2018-10-20T21:54:19Z

I think this existing code is wrong as well. Can someone with more experience please comment on this:

def rcdefaults():
    # [docstring omitted]
    with warnings.catch_warnings():
        warnings.simplefilter("ignore", mplDeprecation)
        from .style.core import STYLE_BLACKLIST
        rcParams.clear()
        rcParams.update({k: v for k, v in rcParamsDefault.items()
                         if k not in STYLE_BLACKLIST})

The defaults should have also STYLE_BLACKLIST parameters. I assume this is what makes the tests fail, because I have now a working clear(). In master the STYLE_BLACKLIST just persisted because clear() was ineffective.

jklymak · 2018-10-22T16:31:26Z

Can you clarify why this is in urgent need of fixing? I don't think its a regression, so I'm pushing to 3.1, but feel free to argue otherwise. I'm also not clear what the practical problem is versus the theoretical problem.

anntzer · 2018-10-23T15:35:31Z

I agree this didn't really qualify as release-critical per se, but it looks like this also fixes #12601, which is release critical, so I'm retargeting...

anntzer · 2018-10-23T15:36:25Z

lib/matplotlib/__init__.py

-if dict.__getitem__(rcParams, 'examples.directory'):
+with warnings.catch_warnings():
+    warnings.simplefilter("ignore", MatplotlibDeprecationWarning)
+    examples_directory = rcParams['examples.directory']


underscore to not make this a publically accessible global

anntzer · 2018-10-23T15:40:45Z

lib/matplotlib/__init__.py

+    def __len__(self):
+        return len(self._data)
+
+    def __delitem__(self, key):


I actually don't think we should allow del'ing items from the rcParams as that has no sense. It may not have been worth it to prevent that with the old implementation, but now it's just a matter of replacing the implementation by a raise TypeError.

Yes, you're right. The current code prevents it, because we did not reimplement the MutableMapping.__delitem__. Before we were just inheriting from dict, so del'ing should have been possible. Inheriting MutableMapping was introduced within a large commit #7549. I dare assuming that both behaviors were incidental and nobody really thought about it.

I'm extending the RcParams in my private code to be able to style and style-context some of my plot elements in specialized plots. While I'm currently not del'ing anything I could imagine that there's a use-case for that. Maybe we should prevent del'ing only for values that are in the defaults.

But as a first step it's ok to stay as restrictive as we currently are.

Talking about custom extensions to rcParams (none of which are really using the public API I guess), I realized that this PR would break https://github.com/anntzer/mplcairo#antialiasing where I use dict.__setitem__ to bypass the validation code on input (to set lines.antialiased to one of the many cairo antialiasing modes, whereas the validator just accepts True/False).
I guess that's related to your point in #12577 (comment)...

I was hoping nobody would mess with the implementation details of RcParams in such a way except our own code. Ok, then we have to take it a step slower, define an official API for that and only later migrate to the MutableMapping.

By the way, What was your reason for introducing the multiple inheritance in the first place? IMO deriving from dict is not good but still far better than deriving from dict and MutableMapping.

I was hoping nobody would mess with the implementation details of RcParams in such a way except our own code. Ok, then we have to take it a step slower, define an official API for that and only later migrate to the MutableMapping.

To be fully consistent with my own general approach wrt backcompat ("we should consider whether it'll actually matter to enough people that we reject a backcompat break"): I'm not going to throw a fit if the API changes here, and having a "more official" way to bypass the validator would be better anyways.

By the way, What was your reason for introducing the multiple inheritance in the first place? IMO deriving from dict is not good but still far better than deriving from dict and MutableMapping.

When I wrote it, it seemed to be a good way to provide update() cheaply.
In fact, looking at it again:

I guess you can still make it just inherit from dict, keeping most of the old code, and then do update = MutableMapping.update (thus easily stealing the implementation from MutableMapping without pulling in the whole multiple inheritance problem, and not breaking my approach in mplcairo :p);

I think the fact that len(rcParams) == 0, i.e. that calls MutableMapping.__len__, is a CPython bug: https://bugs.python.org/issue35063

anntzer · 2018-10-23T15:54:21Z

re: rcdefaults(): I guess you can just get rid of the call to clear()?

tacaswell · 2018-10-23T23:54:37Z

Fixed #12601 with a much smaller change, moved this back to 3.1.

The defaults should have also STYLE_BLACKLIST parameters.

The logic (iirc) was that anything that was not style should not be changed by the restore-default methods.

tacaswell · 2022-12-05T23:22:34Z

@timhoffm What do you want to do about this?

I took a pass at rebasing but still have a few failing tests but nothing that seems in tractable. I am not sure it is worth the code thrashing that this requires, but can be convinced otherwise.

timhoffm · 2022-12-06T09:44:17Z

We should definitively move away from constructs like dict.__getitem__(rcParams, "backend") or dict.update(rcParams, self._orig) as a workaround to escape the automated logic built into RcParams. These rely on an implementation detail and will make it more difficult to built up a new configuration structure (#24585).

Instead, there should be (private?) functions on rcParams to do this. Given that these implementation details already have leaked out (https://github.com/matplotlib/matplotlib/pull/12577/files#r227997056), we should soon establish that API so that users can move away from dict.*(rcParams). To maintain compatibility for a transition phase we have to delay not inheriting from dict. This needs a bit of redesign.

timhoffm · 2023-05-18T21:49:14Z

Superseeded by #25617.

RcParams should not inherit from dict

3399dc4

timhoffm added the status: work in progress label Oct 20, 2018

timhoffm added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Oct 20, 2018

timhoffm added this to the v3.0.x milestone Oct 20, 2018

jklymak modified the milestones: v3.0.x, v3.1 Oct 22, 2018

anntzer modified the milestones: v3.1, v3.0.x Oct 23, 2018

anntzer reviewed Oct 23, 2018

View reviewed changes

tacaswell modified the milestones: v3.0.x, v3.1 Oct 23, 2018

tacaswell removed the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label Oct 23, 2018

timhoffm mentioned this pull request Nov 4, 2018

RcParams is fundamentally broken #12576

Closed

timhoffm modified the milestones: v3.1.0, v3.2.0 Feb 15, 2019

timhoffm modified the milestones: v3.2.0, v3.3.0 Aug 7, 2019

QuLogic modified the milestones: v3.3.0, v3.4.0 May 2, 2020

jklymak added the status: needs rebase label Jul 23, 2020

jklymak marked this pull request as draft July 23, 2020 16:08

QuLogic modified the milestones: v3.4.0, v3.5.0 Jan 21, 2021

timhoffm modified the milestones: v3.5.0, unassigned Jul 8, 2021

story645 modified the milestones: unassigned, needs sorting Oct 6, 2022

tacaswell mentioned this pull request Dec 5, 2022

Requirements for a new configuration management class #24585

Open

timhoffm mentioned this pull request Dec 14, 2022

Data access API for rcParams #24730

Merged

chahak13 mentioned this pull request Apr 13, 2023

Proposal: Implement RcParams using ChainMap and remove dict inheritance #25617

Draft

6 tasks

chahak13 mentioned this pull request May 11, 2023

[Bug]: rcdefaults and rcParams.clear() don't work? #25855

Open

timhoffm closed this May 18, 2023

timhoffm deleted the rcparams-dict branch July 19, 2024 11:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

RcParams should not inherit from dict #12577

RcParams should not inherit from dict #12577

timhoffm commented Oct 20, 2018

timhoffm commented Oct 20, 2018

timhoffm commented Oct 20, 2018 •

edited

Loading

jklymak commented Oct 22, 2018

anntzer commented Oct 23, 2018

anntzer Oct 23, 2018

anntzer Oct 23, 2018

timhoffm Oct 24, 2018

anntzer Oct 24, 2018

timhoffm Oct 25, 2018 •

edited

Loading

anntzer Oct 25, 2018

timhoffm Oct 25, 2018

anntzer commented Oct 23, 2018

tacaswell commented Oct 23, 2018

tacaswell commented Dec 5, 2022

timhoffm commented Dec 6, 2022

timhoffm commented May 18, 2023

RcParams should not inherit from dict #12577

RcParams should not inherit from dict #12577

Conversation

timhoffm commented Oct 20, 2018

PR Summary

timhoffm commented Oct 20, 2018

timhoffm commented Oct 20, 2018 • edited Loading

jklymak commented Oct 22, 2018

anntzer commented Oct 23, 2018

anntzer Oct 23, 2018

Choose a reason for hiding this comment

anntzer Oct 23, 2018

Choose a reason for hiding this comment

timhoffm Oct 24, 2018

Choose a reason for hiding this comment

anntzer Oct 24, 2018

Choose a reason for hiding this comment

timhoffm Oct 25, 2018 • edited Loading

Choose a reason for hiding this comment

anntzer Oct 25, 2018

Choose a reason for hiding this comment

timhoffm Oct 25, 2018

Choose a reason for hiding this comment

anntzer commented Oct 23, 2018

tacaswell commented Oct 23, 2018

tacaswell commented Dec 5, 2022

timhoffm commented Dec 6, 2022

timhoffm commented May 18, 2023

timhoffm commented Oct 20, 2018 •

edited

Loading

timhoffm Oct 25, 2018 •

edited

Loading