Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Do not check value of parameters from rcParam (and check rcParam befo…#26167
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
base:main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Just one question.
Uh oh!
There was an error while loading.Please reload this page.
| if style is None: | ||
| style = mpl.rcParams['font.style'] | ||
| _api.check_in_list(['normal', 'italic', 'oblique'], style=style) | ||
| else: | ||
| _api.check_in_list(['normal', 'italic', 'oblique'], style=style) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I'm not a fan of this kind of change. The code get's longer, harder to follow and more prone to errors.
Double checking the rcParam value here does not hurt. The only disadvantage are 300ns performance loss (which I claim to be negigible in any real-world application). But since the rcParam validation is an independent concept, we don't have a guarantee that they are synchronous. Such differences could be an oversight. But I can also imagine cases where rcParams are used in multiple places and have a slightly broader acceptance range than what a specific function supports.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The thing is that those 300 ns are multiplied with a huge number in many situations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
What is huge? If huge * 300ns > 2% of the total plot time, we may start to consider, otherwise, the benefit is negligible.
Uh oh!
There was an error while loading.Please reload this page.
…re using them)
Inspired by some changes in#26164, but separated as they are not directly connected to that (some that could have been here might have gone in there though...).
If we get the rcParam (and that is checked) or set an argument to a valid value, it is not required to check with
check_in_list.Some rcParams were not checked when loaded, only when used.
Finally, small methods where the likely thing is that the correct parameter is passed and there are if-claused anyway, are checked at the end rather than the beginning.
PR summary
PR checklist