Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

RcParams should not inherit from dict#12577

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
timhoffm wants to merge1 commit intomatplotlib:mainfromtimhoffm:rcparams-dict

Conversation

timhoffm
Copy link
Member

PR Summary

Fixes#12576.

@timhoffm
Copy link
MemberAuthor

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

@timhoffm
Copy link
MemberAuthor

timhoffm commentedOct 20, 2018
edited
Loading

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 defaultsshould have also STYLE_BLACKLIST parameters. I assume this is what makes the tests fail, because I have now a workingclear(). In master the STYLE_BLACKLIST just persisted becauseclear() was ineffective.

@timhoffmtimhoffm added the Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. labelOct 20, 2018
@timhoffmtimhoffm added this to thev3.0.x milestoneOct 20, 2018
@jklymak
Copy link
Member

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.

@jklymakjklymak modified the milestones:v3.0.x,v3.1Oct 22, 2018
@anntzer
Copy link
Contributor

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

@anntzeranntzer modified the milestones:v3.1,v3.0.xOct 23, 2018
if dict.__getitem__(rcParams, 'examples.directory'):
with warnings.catch_warnings():
warnings.simplefilter("ignore", MatplotlibDeprecationWarning)
examples_directory = rcParams['examples.directory']
Copy link
Contributor

Choose a reason for hiding this comment

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

underscore to not make this a publically accessible global

def __len__(self):
return len(self._data)

def __delitem__(self, key):
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Yes, you're right. The current code prevents it, because we did not reimplement theMutableMapping.__delitem__. Before we were just inheriting fromdict, so del'ing should have been possible. InheritingMutableMapping 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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Talking about custom extensions to rcParams (none of which are really using the public API I guess), I realized that this PR would breakhttps://github.com/anntzer/mplcairo#antialiasing where I usedict.__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)...

Copy link
MemberAuthor

@timhoffmtimhoffmOct 25, 2018
edited
Loading

Choose a reason for hiding this comment

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

I was hoping nobody would mess with the implementation details ofRcParams 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 theMutableMapping.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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 fromdict, keeping most of the old code, and then doupdate = 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 callsMutableMapping.__len__, is a CPython bug:https://bugs.python.org/issue35063

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

LGTM

@anntzer
Copy link
Contributor

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

@tacaswelltacaswell modified the milestones:v3.0.x,v3.1Oct 23, 2018
@tacaswelltacaswell removed the Release criticalFor bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. labelOct 23, 2018
@tacaswell
Copy link
Member

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

The defaultsshould have also STYLE_BLACKLIST parameters.

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

@timhoffmtimhoffm modified the milestones:v3.1.0,v3.2.0Feb 15, 2019
@timhoffmtimhoffm modified the milestones:v3.2.0,v3.3.0Aug 7, 2019
@QuLogicQuLogic modified the milestones:v3.3.0,v3.4.0May 2, 2020
@jklymakjklymak marked this pull request as draftJuly 23, 2020 16:08
@QuLogicQuLogic modified the milestones:v3.4.0,v3.5.0Jan 21, 2021
@timhoffmtimhoffm modified the milestones:v3.5.0,unassignedJul 8, 2021
@story645story645 modified the milestones:unassigned,needs sortingOct 6, 2022
@tacaswell
Copy link
Member

@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
Copy link
MemberAuthor

We should definitively move away from constructs likedict.__getitem__(rcParams, "backend") ordict.update(rcParams, self._orig) as a workaround to escape the automated logic built intoRcParams. 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 onrcParams 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 fromdict.*(rcParams). To maintain compatibility for a transition phase we have to delay not inheriting from dict. This needs a bit of redesign.

@timhoffm
Copy link
MemberAuthor

Superseeded by#25617.

@timhoffmtimhoffm deleted the rcparams-dict branchJuly 19, 2024 11:30
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@anntzeranntzeranntzer left review comments

Assignees
No one assigned
Projects
None yet
Milestone
future releases
Development

Successfully merging this pull request may close these issues.

RcParams is fundamentally broken
6 participants
@timhoffm@jklymak@anntzer@tacaswell@QuLogic@story645

[8]ページ先頭

©2009-2025 Movatter.jp