expose zoom_to_selected in catalogs plugin api, fix default zoom level #3369
Conversation
cshanahan1
requested review from
rosteen,
javerbukh,
pllim,
kecnry,
bmorris3,
haticekaratay and
gibsongreen
as code owners
github-actions
bot
added
imviz
plugin
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3369 +/- ##
==========================================
- Coverage 88.76% 88.11% -0.65%
==========================================
Files 125 127 +2
Lines 19248 19543 +295
==========================================
+ Hits 17086 17221 +135
- Misses 2162 2322 +160 ☔ View full report in Codecov by Sentry. |
cshanahan1
changed the title
cshanahan1
force-pushed
the
catalog_api
branch
from
7d0b728 to
06b20aa
Compare
| x = [float(coord['x_coord']) for coord in selected_rows] | ||
| y = [float(coord['y_coord']) for coord in selected_rows] | ||
|
|
||
| limits = viewer.state._get_reset_limits() |
There was a problem hiding this comment.
What if the viewer and the image have different dimensions?
I was testing the default Imviz notebook and one case I noticed was the viewer limits were dependent on my browser instance and how I defined the width but the image keeps the same aspect ratio in the viewer when I resize the browser.
Screen.Recording.2024-12-20.at.4.31.48.PM.mov
There was a problem hiding this comment.
I'm honestly not sure what the answer to this is
There was a problem hiding this comment.
You can reproduce where this case causes and issue with the zoom with the following:
catalog = imviz.plugins['Catalog Search']._obj
catalog.open_in_tray()
catalog.catalog.selected = 'Gaia'
catalog.search()
catalog.table.selected_rows = catalog.table.items[:2]
catalog.zoom_to_selected()
There was a problem hiding this comment.
Is the concern just about reproducibility? Or is it somehow failing to zoom to include all selected sources?
There was a problem hiding this comment.
Can we get the limits from the selected_rows instead of the viewer?
There was a problem hiding this comment.
can you see what is being used as padding? And the output of viewer.state._get_reset_limits() (maybe this is a bug there)? My other idea is that this is just an issue with providing x/y limits as opposed to center and radius, since the rectangle of limits we pass is not exactly what ends up adopted.
There was a problem hiding this comment.
I'd still say this is an improvement over the previous behavior and maybe we get it in as is so that its in for the demos and make a tech-debt ticket to make more general in the future? If we're worried, we can hide padding from the user API until then.
There was a problem hiding this comment.
ok i was able to reproduce the issue, very weird.
There was a problem hiding this comment.
I would say this is still an improvement, because the same zoom scenario on main in the example doesn't work at all and the image is totally zoomed out. This is at least in the right area of the image now when it fails after resizing your browser window. We should look into what is happening though.
There was a problem hiding this comment.
After each step inside zoom_to_selected:
limits:
(-0.5, 4733.5, -0.5, 4718.5)
max_dim:
4734.0
padding:
94.68
before applying padding (x_min, x_max) (y_min, y_max)
2202.56701 2589.79442
2243.31321 2716.02305
after applying padding (x_min, x_max) (y_min, y_max)
2107.8870100000004 2684.47442
2148.63321 2810.7030499999996
After Zooming:
viewer.get_limits()
(2156.4823647015282, 2635.8790652984726, 2365.1348456839964, 2594.2014143160036)
viewer.state._get_reset_limits()
(-0.5, 4733.5, -0.5, 4718.5)
(x_1, x_2) (y_1, y_2) of sources:
(2202.56701,2589.79442) (2243.31321, 2716.02305)
This PR exposes 'zoom_to_selected' in the catalogs plugin, and adds test coverage for this functionality.
This also fixes an issue where 'zoom_to_selected' was using a fixed default value of 50 pixels around the source(s), which is inappropriate for small images (including when wcs linked and the image is 10x10 pixels). The default is now to use 2% of the total image size around the bounding box containing the selected points, which felt like a good size in my testing. The 'padding' parameter, which refers to the fraction of the image size to use as the zoom level around sources, is now exposed in 'zoom_to_selected' as well so it can be adjusted if needed.