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

Add text.parse_math rcParams#22556

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

Merged
QuLogic merged 1 commit intomatplotlib:mainfromoscargus:parse_math_rcparams
Mar 29, 2022

Conversation

oscargus
Copy link
Member

PR Summary

Based on the discussion in#22537, there was a question about controllingparse_math through rcParams. Assuming that one often would like to use some special packages, it probably would be quite convenient to do that.

Should this be documented as a new feature?

(It is not obvious to me how the default value is controlled...)

PR Checklist

Tests and Styling

  • Has pytest style unit tests (andpytest passes).
  • IsFlake 8 compliant (installflake8-docstrings and runflake8 --docstring-convention=all).

Documentation

  • New features are documented, with examples if plot related.
  • New features have an entry indoc/users/next_whats_new/ (follow instructions in README.rst there).
  • [N/A] API changes documented indoc/api/next_api_changes/ (follow instructions in README.rst there).
  • Documentation is sphinx and numpydoc compliant (the docs shouldbuild without error).

@tacaswelltacaswell added this to thev3.6.0 milestoneMar 1, 2022
@oscargusoscargusforce-pushed theparse_math_rcparams branch 2 times, most recently fromb7dbd28 to6deed94CompareMarch 2, 2022 12:51
@oscargus
Copy link
MemberAuthor

Just to confirm, in general the preferred approach is as.

In__init__(..., parameter=None, ...):

self._parameter = None self.set_parameter(parameter)

Inset_parameter:

...     parameter : type, default: :rc:`...`...if parameter is None:    self._parameter = rcParams...else:    self._parameter = parameter

?

I noted that this didn't really hold in all files. Main benefit, I guess, is that the default value is handled consistently both in documentation and in the code (so that it is "always" possible to call with None to reset to default) and so that the redundant calls to rcParam are reduced.

@oscargus
Copy link
MemberAuthor

Modified the test to use rc_context (which I assume is the better way?).

@timhoffm
Copy link
Member

timhoffm commentedMar 6, 2022
edited
Loading

Modified the test to use rc_context (which I assume is the better way?).

Each test gets fresh rcParams to ensure they are isolated. So it doesn't really matter here.

Just to confirm, in general the preferred approach is as.

We do not have a policy on this. This approach makes the setters to acceptNone, meaning "set to rcParams". This is a bit of a stretch of the setter concept, but there's precedence, so it's okish.

@oscargus
Copy link
MemberAuthor

This is a bit of a stretch of the setter concept.

In a good or a bad way? 😄

Just noted that this was heavily used in the rest of this file (at least).

(Saw on SE (although I cannot find it now) that theself._parameter = None in__init__ was not really encouraged. I seem to recall that some static code analysis tools are complaining, but maybe they realize that it in fact is initialized when__init__ is called.)

@oscargus
Copy link
MemberAuthor

Finally got around to updating this. Now with a minimal impact on the code, simply checking on creation.

@timhoffm
Copy link
Member

I think we should have another reviewer because the PR changed significantly after@tacaswell's approval.

@QuLogicQuLogic merged commitdd3b02c intomatplotlib:mainMar 29, 2022
@oscargusoscargus deleted the parse_math_rcparams branchMarch 29, 2022 06:30
@QuLogicQuLogic mentioned this pull requestSep 9, 2022
2 tasks
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@tacaswelltacaswelltacaswell approved these changes

@QuLogicQuLogicQuLogic approved these changes

@timhoffmtimhoffmtimhoffm approved these changes

Assignees
No one assigned
Projects
None yet
Milestone
v3.6.0
Development

Successfully merging this pull request may close these issues.

4 participants
@oscargus@timhoffm@tacaswell@QuLogic

[8]ページ先頭

©2009-2025 Movatter.jp