Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Add _val_or_rc-function#26168
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
Add _val_or_rc-function#26168
Uh oh!
There was an error while loading.Please reload this page.
Conversation
3cfb5fc tobc1658eCompareThere 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.
Becauselegend.val_or_rc is formally public API, it probably should be removed after a standard deprecation cycle.
I do not think that? I think it is an internal function in |
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.
Oops! You are right, of course. All set.
| @@ -603,11 +592,9 @@ def val_or_rc(val, rc_name): | |||
| 'markeredgecolor': ['get_markeredgecolor', 'get_edgecolor'], | |||
| 'mec': ['get_markeredgecolor', 'get_edgecolor'], | |||
| } | |||
| labelcolor = mpl._val_or_rc(labelcolor, 'legend.labelcolor') | |||
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.
You could nest anothermpl._val_or_rc(..., 'text.color') here and add a short comment about falling back totext.color iflegend.labelcolor is not present.
| def val_or_rc(val, rc_name): | ||
| return val if val is not None else mpl.rcParams[rc_name] |
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 think you could possibly save a bunch of method gets below by setting this and reusing it?val_or_rc = mpl._val_or_rc
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 would like to warn on ineffective micro optimizations. Squeezing out nanoseconds does not make any measureable difference.
A function lookup cost 30ns (Python 3.11):
In [2]: %timeit mpl.rc_params_from_file29.7 ns ± 0.544 ns per loopIf we want to improve performance (and there's a lot to gain) that would need to be guided by profiling to identify the real pain points. In other cases, code structure/readability should be the primary guideline. - Not shure what is better here, but I tend to not do the additional logic indirection for saving a method to a new variable just to save method lookups.
| @@ -1294,6 +1294,13 @@ def is_interactive(): | |||
| return rcParams['interactive'] | |||
| def _val_or_rc(val, rc_name): | |||
| """ | |||
| If *val* is None, return ``mpl.rcParams[rc_name]``, otherwise return val. | |||
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.
| If*val*isNone,return``mpl.rcParams[rc_name]``,otherwisereturnval. | |
| Returnthevalueifpresent,otherwiselookup`rc_name`inthercParamsdictionary. |
I'd suggest rewording this to be more human friendly since the code already explains what you had written.
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.
Using None andmpl.rcParams[rc_name] is good, because that's precise and comprehensible. Optional, slightly less algorithmic variant.
| If*val*isNone,return``mpl.rcParams[rc_name]``,otherwisereturnval. | |
| Pass*val*throughunlessitisNone,inwhichcasereturn``mpl.rcParams[rc_name]``. |
| @@ -1294,6 +1294,13 @@ def is_interactive(): | |||
| return rcParams['interactive'] | |||
| def _val_or_rc(val, rc_name): | |||
| """ | |||
| If *val* is None, return ``mpl.rcParams[rc_name]``, otherwise return val. | |||
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.
Using None andmpl.rcParams[rc_name] is good, because that's precise and comprehensible. Optional, slightly less algorithmic variant.
| If*val*isNone,return``mpl.rcParams[rc_name]``,otherwisereturnval. | |
| Pass*val*throughunlessitisNone,inwhichcasereturn``mpl.rcParams[rc_name]``. |
I think an accident during confict resolution ofmatplotlib#26168 incorrectlyoverwrote antialias settings for MathText.Also, re-arrange the tests to better catch this error.
I think an accident during confict resolution ofmatplotlib#26168 incorrectlyoverwrote antialias settings for MathText. This puts the `_val_or_rc`call in the correct location.Also, re-arrange the tests to better catch this error.
Uh oh!
There was an error while loading.Please reload this page.
PR summary
is a very common code pattern, so may be good to have a helper for it.
Right now I only consideredlegend.py, but if this is an accepted idea there are many places where it can be used.(And, yes, the reason I started with legend is that it was defined as a local function there.)
PR checklist