Imviz: Add zoom and zoom_level API #744
Conversation
pllim
force-pushed
the
imviz-zoom-not-the-meeting
branch
from
912923b to
ced8ee0
Compare
pllim
force-pushed
the
imviz-zoom-not-the-meeting
branch
from
ced8ee0 to
a39b66a
Compare
Codecov Report
@@ Coverage Diff @@
## main #744 +/- ##
==========================================
+ Coverage 63.00% 63.09% +0.08%
==========================================
Files 65 65
Lines 4290 4319 +29
==========================================
+ Hits 2703 2725 +22
- Misses 1587 1594 +7
Continue to review full report at Codecov.
|
pllim
force-pushed
the
imviz-zoom-not-the-meeting
branch
from
fa852b0 to
d3c7f37
Compare
pllim
requested review from
duytnguyendtn,
ibusko,
javerbukh and
rosteen
as code owners
|
|
||
| # NOTE: Not sure why X/Y min/max not exactly the same as aspect ratio 1 | ||
| self.imviz.zoom_level = 1 | ||
| self.assert_zoom_results(1, -46, 54, -45.5, 54.5, dpix) |
There was a problem hiding this comment.
Does seem weird some x/y min/max not matching exactly for square data and display at aspect ratio one, but that adjustment is deep within glue core and I don't feel like digging no more. If @astrofrog has ideas, then great. Otherwise, I'll just leave it be.
| # in the unit test when off-center. Works in notebook though. | ||
| if not is_offcenter: | ||
| self.imviz.zoom_level = 'fit' | ||
| self.assert_zoom_results(10, -0.5, 9.5, -0.5, 9.5, 0) |
There was a problem hiding this comment.
For the off-center case, local testing showed that y_min and y_max was -5. , 5. instead of -0.5, 9.5 like X. Something not triggering here in the way I set up or hack around the viewer for unit testing. I swear to you it works properly in a notebook. If @astrofrog has ideas, then great. If not, I'll just leave this untested for the off-center case.
There was a problem hiding this comment.
Made one change request, although I haven't been part of the astrowidgets API discussion so maybe there was a reason for not making zoom have the input option I suggested there. Other than that I think this looks good - I don't have any insight into why your test cases are weird for Y, but it seems to be working as I would expect in the notebook.
| """ | ||
| if not isinstance(val, (int, float)): | ||
| raise ValueError(f"zoom only accepts int or float but got '{val}'") | ||
| self.zoom_level = self.zoom_level * val |
There was a problem hiding this comment.
If this is simply a method to set the zoom_level, it would would be nice to accept "fit" as input here as well as when setting zoom_level directly. That was one of the first things I tried and I was surprised that it didn't work.
There was a problem hiding this comment.
That would go against the current astrowidgets API upstream though...
There was a problem hiding this comment.
zoom() was supposed to be relative zoom, so "fit" does not really fit (pun?) in that paradigm.
There was a problem hiding this comment.
Ah I see, that's the context I was missing, I guess. I didn't fully appreciate that zoom_level is setting the absolute zoom relative to the image, whereas zoom is setting the zoom relative to the current display.
There was a problem hiding this comment.
I can't blame you. I didn't either until astropy/astrowidgets#144
|
Re: gripes -- Will be happy to get your feedback over at astropy/astrowidgets#142 ❗ |
|
I didn't hear any crying or objections during sprint review. Does that mean we can merge this? |
|
I think I consider a live demo with general approval as a second thumbs up. Merging. |
Fix #715
TODO
viewer.shape