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

switch order of setting ymin and ymax #3371

Merged
merged 4 commits into from
Dec 23, 2024

Conversation

cshanahan1
Copy link
Contributor

@cshanahan1 cshanahan1 commented Dec 23, 2024

This fixes an issue where back-to-back calls to set_limits and get_limits were producing different results for y limits. For example,

viewer.set_limits(x_min=0, x_max=20, y_min=0, y_max=20)
limits = viewer.get_limits()
(0, 20.0, 3.855, 16.145).

For whatever reason, switching the order that ymin and ymax are set fixes this issue. I'm not sure if this entirely fixes the zoom/resize issue noted in #3369, but it resolves this round tripping that should work. There is still a deeper issue going on here.

@rosteen
Copy link
Collaborator

rosteen commented Dec 23, 2024

I tried running this test on main and it also passes there - is there a way to make one that fails on main?

@cshanahan1
Copy link
Contributor Author

cshanahan1 commented Dec 23, 2024

Huh, the same test fails in a notebook on main. Maybe this requires the image to actually be rendered?

edit: The test does pass in the notebook if i don't do imviz.show()

@rosteen
Copy link
Collaborator

rosteen commented Dec 23, 2024

Hm, yeah, seems like it requires actual rendering of the app then. You can probably just remove the test then, no need to run it if it's not checking anything? 🤷

Copy link

codecov bot commented Dec 23, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.11%. Comparing base (6868bb5) to head (5445a76).
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3371   +/-   ##
=======================================
  Coverage   88.11%   88.11%           
=======================================
  Files         127      127           
  Lines       19574    19574           
=======================================
  Hits        17248    17248           
  Misses       2326     2326           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Collaborator

@rosteen rosteen left a comment

Choose a reason for hiding this comment

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

I'm fine with merging this, but it perhaps warrants a deeper investigation at some point about why this matters.

@rosteen rosteen self-requested a review December 23, 2024 16:07
@rosteen rosteen added the trivial Only needs one approval instead of two label Dec 23, 2024
@cshanahan1 cshanahan1 merged commit bcf44af into spacetelescope:main Dec 23, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
imviz trivial Only needs one approval instead of two
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants