| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
Sorry, something went wrong.
|
I agree that one should probably not copy the arrowprops part, but maybe something like arrowprops: dict, optional
Arrow properties, see `.Annotation` for description.
fontsize: float or str, optional
Font size, see `~Text.set_fontsize`.
? As these properties are explicit in the constructor. (fontsize is documented as part of **kwargs in Annotation as "Additional kwargs are passed to Text" and not directly documented in Text either... One may wonder why it is explicitly mentioned in AnnotationBbox. Turns out that annotation_clip is also missing... Anyway, no strong opinions as such (I can always add those later), but I'll leave it for a while to see if someone has stronger opinions. |
Sorry, something went wrong.
|
Now I look deeper, I think having "Other parameters are identical to Annotation" next to **kwargs here is also wrong, or at least misleading. It implies that this **kwargs would include Annotation's **kwargs, which are passed to Text, as you say. But AnnotationBbox's **kwargs are passed to self._internal_update. So maybe the **kwargs description here should reference e.g. the set method? |
Sorry, something went wrong.
|
I think I have addressed all the comments, but... What text does this fontsize keyword apply to? If I put e.g. fontsize="xx-large" in the call to AnnotationBbox in my example at #24390, it doesn't seem to make a difference. That makes sense, as you would set the fontsize for the text in the box using the TextArea object instead. Is there some other text associated with the AnnotationBbox? |
Sorry, something went wrong.
Looking into the code, the fontsize parameter is stored in a FontPropertiesobject at AnnotationBbox.prop and its only use is to define the mutation_scale parameter of the FancyBboxPatch, i.e. affecting attributes of the box style like padding and rounding of corners (if defined). AFAICS we should rename fontsize to mutation_scale and deprecate prop. But ping @anntzer, who wrote this code in 2009. |
Sorry, something went wrong.
|
I didn't really look at what piece of code you're talking about (can you point out the line?) but I was certainly not involved with matplotlib development yet in 2009... |
Sorry, something went wrong.
|
I think mutation_scale can be passed as part of bboxprops, so we could just deprecate fontsize and direct the user to use bboxprops=dict(mutation_scale=x) instead? It would seem odd to me to have mutation_scale accessible at the top level when the other FancyBboxPatch parameters (like boxstyle) are passed within the dictionary. matplotlib/lib/matplotlib/offsetbox.py Lines 1296 to 1305 in 1fa7467 |
Sorry, something went wrong.
Sorry, mixed up users. - Just read the short version "09.08.09 Lee", but it was @leejjoon. @leejjoon do you rember the motivation in AnnotationBbox to pass fontsize -> self.prop -> FancyBboxPatch(..., mutation_scale=...)? matplotlib/lib/matplotlib/offsetbox.py Line 1235 in 1fa7467 matplotlib/lib/matplotlib/offsetbox.py Lines 1348 to 1358 in 1fa7467 matplotlib/lib/matplotlib/offsetbox.py Lines 1296 to 1302 in 1fa7467 it indeed seems that mutation_scale can be passed already via bboxprops and the fontsize parameter is not needed at all. |
Sorry, something went wrong.
|
I guess using fontsize gives the user the convenience of passing a string, rather than points. Also it currently defaults to rcParams["legend.fontsize"]. If we left it to the FancyBBoxPatch default of 1, it would presumably look quite cramped. So perhaps using fontsize is the most practical way to get a nice looking pad, etc., even though we are not dealing with any actual fonts. Edit: AnchoredOffsetbox does something similar with the prop parameter. |
Sorry, something went wrong.
|
My vague recollection is that I wanted to copy the behavior from annotation (and text) that the mutation scale is set to their fontsize. As @rcomer commented in his post, I think this was a sensible thing to do. The parameter name of fontsize could be misleading and may need to be replaced. It would be good if we can keep the current behavior, though. |
Sorry, something went wrong.
|
@timhoffm I have updated the fontsize description to explain how it is used. Do you think this is enough? Once we understand the reasoning, the name does not seem wrong to me. Perhaps a little unintuitive, but I wonder if that matters enough to go through the breaking change procedure. |
Sorry, something went wrong.
There was a problem hiding this comment.
@timhoffm I have updated the fontsize description to explain how it is used. Do you think this is enough? Once we understand the reasoning, the name does not seem wrong to me. Perhaps a little unintuitive, but I wonder if that matters enough to go through the breaking change procedure.
I still think the name is misleading because it does not affect any font. It only happens to be that mutation_scale is defined in such a way that setting mutation_scale to the used fontsize gives a reasonable scaling (e.g. for pad). That could also be done with an explicit name mutation_scale, including supporting string values.
That said, I would not name it like this again, but I'm not dogmatic about changing the name. When we have at least documented what the parameter is about that's already a big improvement. But if we keep the name AnnotationBbox.set_fontsize needs a similar description.
Sorry, something went wrong.
|
|
||
| annotation_clip: bool or None, default: None | ||
| Whether to clip (i.e. not draw) the annotation when the annotation | ||
| point *xy* is outside the axes area. see `.Annotation` for |
There was a problem hiding this comment.
I think it's better to copy the whole description from Annotation. It's not likely to change, and there is value in having the explicit value description here. Not least because the docstring of Annotation is long and it's a bit cumbersome to find the relevant section.
Sorry, something went wrong.
| `.FancyBboxPatch` to scale attributes of the box style (e.g. pad | ||
| or rounding_size). Passed here as *fontsize* for the convenience | ||
| of using the named sizes. If not given, :rc:`legend.fontsize` is | ||
| used. See `~.Text.set_fontsize` for valid values. |
There was a problem hiding this comment.
| used. See `~.Text.set_fontsize` for valid values. | |
| used. See `.Text.set_fontsize` for valid values. |
so that it renders as Text.set_fontsize and not set_fontsize and by that make it very clear that its not AnnotationBbox.set_fontsize`.
Sorry, something went wrong.
| or rounding_size). Passed here as *fontsize* for the convenience | ||
| of using the named sizes. If not given, :rc:`legend.fontsize` is |
There was a problem hiding this comment.
| or rounding_size). Passed here as *fontsize* for the convenience | |
| of using the named sizes. If not given, :rc:`legend.fontsize` is | |
| or rounding_size). If not given, :rc:`legend.fontsize` is |
I'd leave this out. I think it's not really helpful for understanding (or even technically necessary to provide the convenience).
If you want to comment on the name, it should be more in the direction
the name is chosen in analogy to [...*] where fontsize defines the mutation scale as well.
* To be checked what exactly: Text/Annotation/Legend/FancyBboxPatch.
Sorry, something went wrong.
There was a problem hiding this comment.
Looks like it's in Text
matplotlib/lib/matplotlib/text.py
Line 582 in 57710db
Sorry, something went wrong.
|
|
||
| **kwargs | ||
| Other parameters are identical to `.Annotation`. | ||
| Other `AnnotationBbox` properties. See `~.AnnotationBbox.set` for |
There was a problem hiding this comment.
| Other `AnnotationBbox` properties. See `~.AnnotationBbox.set` for | |
| Other `AnnotationBbox` properties. See `.AnnotationBbox.set` for |
Minor, but I think it's slightly better to display the class here as well.
Sorry, something went wrong.
|
Thanks @rcomer! I appreciate this work. It's often more cumbersome than initially expected, but it really makes our docs much better. |
Sorry, something went wrong.
…444-on-v3.6.x Backport PR #24444 on branch v3.6.x (DOC: AnnotationBbox keyword descriptions)
| Back | FazBrowse Home | New Git URL |
PR Summary
Closes #24390.
PR Checklist
Tests and Styling
Documentation
Release Notes