Skip to content
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

Avoid interference of ROI patches with legend auto-placement #2370

Merged
merged 4 commits into from
Mar 21, 2023

Conversation

dhomeier
Copy link
Collaborator

@dhomeier dhomeier commented Mar 1, 2023

Description

Reported in #2368; as of matplotlib 3.7 legend auto-placement is affected by the position of ROIs, even if empty. Notably 4 tests in viewers/scatter failed as the default placement of a MplPolygonalROI pushed the legends out of their usual position.
As a workaround the default extent and position of the ROI is moved from [0, 1], [0, 1] to [0.2, 0.3], [0.2, 0.3] to keep out of the way of any of the 9 standard positions for the legend. Better fix would be to hold off syncing the patch until the ROI is made visible. Alternatively xfail for matplotlib >= 3.7.

@dhomeier dhomeier marked this pull request as ready for review March 2, 2023 17:11
@dhomeier
Copy link
Collaborator Author

dhomeier commented Mar 3, 2023

@Carifio24, @astrofrog I have tried to implement the suggested approach from #2368 (comment) in _sync_patch now and putting this up for review as such.
While tests are passing, it might be good to check the behaviour in the app – my installation on Apple M1 does not play very nicely with adding and manipulating several ROIs.

glue/core/roi.py Outdated Show resolved Hide resolved
@dhomeier dhomeier force-pushed the i-shrunk-the-patch branch 3 times, most recently from 372e4a7 to 759dee9 Compare March 21, 2023 15:17
@dhomeier dhomeier changed the title WIP: avoid interference of ROI patches with legend placement Avoid interference of ROI patches with legend auto-placement Mar 21, 2023
@dhomeier
Copy link
Collaborator Author

Thanks for your assistance @Carifio24!

@dhomeier dhomeier merged commit e71d52d into glue-viz:main Mar 21, 2023
@dhomeier dhomeier deleted the i-shrunk-the-patch branch March 21, 2023 21:13
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.

2 participants