Conversation
3cfb5fc to
bc1658e
Compare
efiring
left a comment
There was a problem hiding this comment.
Because legend.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 |
efiring
left a comment
There was a problem hiding this comment.
Oops! You are right, of course. All set.
| '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.
You could nest another mpl._val_or_rc(..., 'text.color') here and add a short comment about falling back to text.color if legend.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.
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.
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_file
29.7 ns ± 0.544 ns per loop
If 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.
|
|
||
| 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.
| If *val* is None, return ``mpl.rcParams[rc_name]``, otherwise return val. | |
| Return the value if present, otherwise lookup `rc_name` in the rcParams dictionary. |
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.
Using None and mpl.rcParams[rc_name] is good, because that's precise and comprehensible. Optional, slightly less algorithmic variant.
| If *val* is None, return ``mpl.rcParams[rc_name]``, otherwise return val. | |
| Pass *val* through unless it is None, in which case return ``mpl.rcParams[rc_name]``. |
|
|
||
| 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.
Using None and mpl.rcParams[rc_name] is good, because that's precise and comprehensible. Optional, slightly less algorithmic variant.
| If *val* is None, return ``mpl.rcParams[rc_name]``, otherwise return val. | |
| Pass *val* through unless it is None, in which case return ``mpl.rcParams[rc_name]``. |
I think an accident during confict resolution of matplotlib#26168 incorrectly overwrote antialias settings for MathText. Also, re-arrange the tests to better catch this error.
I think an accident during confict resolution of matplotlib#26168 incorrectly overwrote 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.
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