Skip to content

Cleanups to Annotation. #25940

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
May 23, 2023
Merged

Cleanups to Annotation. #25940

merged 1 commit into from
May 23, 2023

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented May 21, 2023

  • Reorder logic in _get_xy_transform to make the case of xy0 not being specified (i.e. invalid bbox_name) fail more obviously.
  • Document "offset fontsize" option.

See discussion at #25907.

PR summary

PR checklist

Copy link
Member

@story645 story645 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit that I expect you'll tell me to implement in a follow up.

'offset points' Offset (in points) from the *xy* value
'offset pixels' Offset (in pixels) from the *xy* value
================= =========================================
'offset fontsize' Offset (relative to fontsize) from the *xy* value
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slightly unclear what relative to font size means here - I can do a followup on the annotations tutorial to add an offset font size example, but can this be in some form similar to "offset in x relative to font size from the xy value"

And yeah I disagree with the parenthesis here b/c in this whole chunk the unit is the important bit, would probably rework table so that the header is "value | unit of offset from xy "

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What other font size there is? This is literally in the docstring for annotate() which is about putting a text somewhere.
I did remove the parentheses, though.
I do think keeping the same header as for xycoords is better, as this table is supposed to be a continuation of the one just above.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I mean is what does relative to font size mean - like if I set offset to (12,12) in font size, what does that mean for how far the offset is from the text?.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If your font size is 12pt, then 1 means "12 pt".

- Reorder logic in _get_xy_transform to make the case of xy0 not being
  specified (i.e. invalid bbox_name) fail more obviously.
- Document "offset fontsize" option.
@oscargus
Copy link
Member

fontsize can be used for figure, subfigure and axes as well? Should it be a follow-up PR to add docs for those or will you handle it here as well?

Also, I think it would be nice to a add a test for some fontsize variant, although I understand that nothing has changed.

Is scale(1) an expensive op? If not, one may consider setting the scale factor in the if-clauses.

@anntzer
Copy link
Contributor Author

anntzer commented May 21, 2023

fontsize can be used for figure, subfigure and axes as well?

I guess so, but I don't think e.g. axes fontsize (some number of fontsizes away from the lower left corner of the axes) is really worth documenting: for the lower left corner it is equivalent to annotate(xy=(0, 0), xycoords="axes fraction", xytext=some_offset, textcoords="offset font"), but the latter form also works for the other corners whereas there's no way to specify the other corners with axes fontsize. So I'd say documenting it is just an attractive nuisance. (We could rework the code to disable that use, but I'm not going to bother doing so here.)

Is scale(1) an expensive op? If not, one may consider setting the scale factor in the if-clauses.

If you're thinking tr = Affine2D().scale(... if unit == ... else ... if unit == ...) etc., I'd say (even though I usually like that kind of constructs) that this is worse 1) because of the star-unpacking in the "fraction" case and 2) because of the need to throw ValueError in the last case (I've always wanted python to have "raise expressions" so that one can write foo = bar if baz else raise Exception(...), but heh...)

I think it would be nice to a add a test for some fontsize variant

At least #25905 will exercise this in building the docs; I'll skip actual tests for now...

@oscargus
Copy link
Member

oscargus commented May 22, 2023

More something like:

        if unit == "points":
            scale = self.figure.dpi / 72  # dpi/72 dots per point
        elif unit == "pixels":
            scale = 1.
        ...

        return Affine2D().scale(scale).translate(*xy0)

@anntzer
Copy link
Contributor Author

anntzer commented May 22, 2023

But there's the unit == "fraction" case, which is a bit ugly to also handle here.

@oscargus
Copy link
Member

But there's the unit == "fraction" case, which is a bit ugly to also handle here.

Fair enough. Didn't see that.

@oscargus oscargus merged commit 8293774 into matplotlib:main May 23, 2023
@anntzer anntzer deleted the an branch May 23, 2023 08:15
@QuLogic QuLogic added this to the v3.8.0 milestone May 23, 2023
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