Skip to content

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 our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 29, 2022
Merged

Conversation

oscargus
Copy link
Member

PR Summary

Based on the discussion in #22537, there was a question about controlling parse_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 (and pytest passes).
  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

Documentation

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

@oscargus oscargus force-pushed the parse_math_rcparams branch from d3d7acb to d5e6ed5 Compare March 1, 2022 09:33
@tacaswell tacaswell added this to the v3.6.0 milestone Mar 1, 2022
@oscargus oscargus force-pushed the parse_math_rcparams branch 2 times, most recently from b7dbd28 to 6deed94 Compare March 2, 2022 12:51
@oscargus
Copy link
Member Author

oscargus commented Mar 3, 2022

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

In __init__(..., parameter=None, ...):

self._parameter = None 
self.set_parameter(parameter)

In set_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 oscargus force-pushed the parse_math_rcparams branch from 6deed94 to efe77be Compare March 6, 2022 12:17
@oscargus
Copy link
Member Author

oscargus commented Mar 6, 2022

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

@timhoffm
Copy link
Member

timhoffm commented Mar 6, 2022

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 accept None, 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
Member Author

oscargus commented Mar 6, 2022

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 the self._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 oscargus force-pushed the parse_math_rcparams branch from b960ac4 to 2023a54 Compare March 28, 2022 13:24
@oscargus
Copy link
Member Author

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.

@QuLogic QuLogic merged commit dd3b02c into matplotlib:main Mar 29, 2022
@oscargus oscargus deleted the parse_math_rcparams branch March 29, 2022 06:30
@QuLogic QuLogic mentioned this pull request Sep 9, 2022
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants