Skip to content

Fixes #20044 pass AnnotationBbox to renderer #24637

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Dec 22, 2022
Merged

Fixes #20044 pass AnnotationBbox to renderer #24637

merged 5 commits into from
Dec 22, 2022

Conversation

saranti
Copy link
Contributor

@saranti saranti commented Dec 6, 2022

PR Summary

Apply the fix from #15087 to the AnnotationBbox so the artist gid associated with each image can be accessed when saved to an svg.

PR Checklist

Documentation and Tests

  • Has pytest style unit tests (and pytest passes)
  • Documentation is sphinx and numpydoc compliant (the docs should build without error).
  • New plotting related features are documented with examples.

Release Notes

  • New features are marked with a .. versionadded:: directive in the docstring and documented in doc/users/next_whats_new/
  • API changes are marked with a .. versionchanged:: directive in the docstring and documented in doc/api/next_api_changes/
  • Release notes conform with instructions in next_whats_new/README.rst or next_api_changes/README.rst

@QuLogic
Copy link
Member

QuLogic commented Dec 6, 2022

Note, issue linking belongs in the PR description or within the commit message, not the PR/commit title.

dstansby
dstansby previously requested changes Dec 6, 2022
View reviewed changes
Copy link
Member

@dstansby dstansby left a comment

Choose a reason for hiding this comment

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

Thanks for opening the PR! Could you add a test to make sure the gid is set for an AnnotationBbox to make sure this doesn't break again in the future? I think the easiest way would be to modify the test added in #15087.

@saranti
Copy link
Contributor Author

saranti commented Dec 6, 2022

Thanks for opening the PR! Could you add a test to make sure the gid is set for an AnnotationBbox to make sure this doesn't break again in the future? I think the easiest way would be to modify the test added in #15087.

Sure thing 👍

@tacaswell tacaswell added this to the v3.7.0 milestone Dec 7, 2022
tacaswell
tacaswell reviewed Dec 8, 2022
View reviewed changes
lib/matplotlib/tests/test_backend_svg.py Outdated

fig = plt.figure()
ax = fig.add_subplot()
arr_img = plt.imread("annotationbbox_gid.png")
Copy link
Member

Choose a reason for hiding this comment

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

Can you use one of our existing images?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see why not

greglucas
greglucas reviewed Dec 13, 2022
View reviewed changes
lib/matplotlib/tests/test_backend_svg.py Outdated
def test_annotationbbox_gid():
# Test that object gid appears in the AnnotationBbox
# in output svg.
from matplotlib.offsetbox import (OffsetImage, AnnotationBbox)
Copy link
Contributor

Choose a reason for hiding this comment

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

imports should be at the top of the file

lib/matplotlib/tests/test_backend_svg.py Outdated
Comment on lines 598 to 599
png_path = Path(__file__).parent / \
"baseline_images\test_backend_svg\noscale.png"
Copy link
Contributor

Choose a reason for hiding this comment

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

Paths are forward "/" on non-windows systems. Since you're already using Path though you can just keep adding

Suggested change
png_path = Path(__file__).parent / \
"baseline_images\test_backend_svg\noscale.png"
png_path = Path(__file__).parent / \
"baseline_images" / "test_backend_svg" / "noscale.png"

with that being said though, I'm not sure you even need to load an image here since you aren't doing an image comparison? So, perhaps just pass a 2d array to the OffsetImage below.

arr_img = np.ones((32, 32))

@tacaswell tacaswell dismissed dstansby’s stale review December 15, 2022 15:23

Now has a test.

@tacaswell
Copy link
Member

This needs to be rebased / squashed (the failure is that a file was added and then removed in the history so we need to re-write history to avoid that), othewise looks good 👍🏻

tacaswell
tacaswell approved these changes Dec 15, 2022
View reviewed changes
Copy link
Member

@tacaswell tacaswell left a comment

Choose a reason for hiding this comment

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

modulo squashing the commits

QuLogic
QuLogic reviewed Dec 17, 2022
View reviewed changes
QuLogic
QuLogic approved these changes Dec 21, 2022
View reviewed changes
@QuLogic
Copy link
Member

QuLogic commented Dec 22, 2022

This should be squashed when merging.

@QuLogic QuLogic merged commit 937e745 into matplotlib:main Dec 22, 2022
@saranti saranti deleted the setgid branch January 2, 2023 04:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@greglucas greglucas greglucas left review comments

@tacaswell tacaswell tacaswell approved these changes

@QuLogic QuLogic QuLogic approved these changes

@dstansby dstansby dstansby left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v3.7.0
Development

Successfully merging this pull request may close these issues.
5 participants
@saranti @QuLogic @tacaswell @dstansby @greglucas