Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
expose zoom_to_selected in catalogs plugin api, fix default zoom level #3369
base: main
Are you sure you want to change the base?
expose zoom_to_selected in catalogs plugin api, fix default zoom level #3369
Changes from all commits
06b20aa
3c9bc28
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm honestly not sure what the answer to this is
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can reproduce where this case causes and issue with the zoom with the following:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we get the limits from the selected_rows instead of the viewer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok i was able to reproduce the issue, very weird.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)