Fix reparenting subsets between rotation orientations #2734

rosteen · 2024-02-27T22:26:00Z

Previously, the subset reparenting didn't properly handle the rotation between two different orientations. This fixes it. I'll add test coverage tomorrow.

pllim · 2024-02-27T22:54:32Z

You would have to also remove the xfail here:

jdaviz/jdaviz/configs/imviz/tests/test_delete_data.py

Line 79 in 20de93b

@pytest.mark.xfail(reason="FIXME: JDAT-3958")

pllim

Need a change log.

And remove leftover debugging stuff.

jdaviz/app.py

pllim

PEP 8

jdaviz/app.py

… properly

… wrong

codecov · 2024-02-28T22:25:23Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.92%. Comparing base (0336005) to head (65f3e10).
Report is 7 commits behind head on main.

❗ Current head 65f3e10 differs from pull request most recent head b01b2a6. Consider uploading reports for the commit b01b2a6 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2734      +/-   ##
==========================================
+ Coverage   88.67%   88.92%   +0.24%     
==========================================
  Files         108      108              
  Lines       15886    15930      +44     
==========================================
+ Hits        14087    14165      +78     
+ Misses       1799     1765      -34

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

jdaviz/configs/imviz/tests/test_delete_data.py

pllim · 2024-02-29T15:53:59Z

p.s. Looks like I am not off the hook from #2724 since you didn't have to remove the AssertionError from these.

jdaviz/jdaviz/configs/imviz/tests/test_orientation.py

Lines 140 to 145 in 0e0a3b7

    
           # FIXME: spacetelescope/jdaviz#2724 (remove AssertionError) 
        
           lc_plugin.link_type = 'WCS' 
        
           with pytest.raises(AssertionError): 
        
               assert self.viewer.state.reference_data.label == "Default orientation" 
        
           with pytest.raises(AssertionError): 
        
               assert viewer_2.state.reference_data.label == "Default orientation"

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

pllim · 2024-02-29T16:46:03Z

jdaviz/app.py

+                            x1, y1 = pixel_to_pixel(old_parent.coords, new_parent.coords,
+                                                    roi.xmin, roi.ymin)
+                            x2, y2 = pixel_to_pixel(old_parent.coords, new_parent.coords,
+                                                    roi.xmin, roi.ymax)
+                            x3, y3 = pixel_to_pixel(old_parent.coords, new_parent.coords,
+                                                    roi.xmax, roi.ymax)
+                            x3, y3 = pixel_to_pixel(old_parent.coords, new_parent.coords,
+                                                    roi.xmax, roi.ymin)


pixel_to_pixel can vectorize, I think, see https://docs.astropy.org/en/stable/api/astropy.wcs.utils.pixel_to_pixel.html , so you really only need one call to that function.

Hmm, I tried calling it a couple different ways, and it doesn't seem like vectorizing analogously to pixel_to_world works for pixel_to_pixel. I did remove an extra call, realized I was setting then overwriting x3 and y3.

Oh, OK, maybe the API doc is misleading then. Thanks for trying!

pllim · 2024-02-29T16:48:46Z

jdaviz/app.py

@@ -1995,7 +2024,11 @@ def vue_data_item_remove(self, event):

        data_label = event['item_name']
        data = self.data_collection[data_label]
-        self._reparent_subsets(data)
+        if self.config == "imviz":


Also have to check link type here? You don't want this happening when linked by pixel, right? Or is the link type being handled somewhere else?

The check that link type is WCS is in _reparent_subsets.

Can we check something else other than the config? Maybe check if the orientation plugin is loaded (which could probably be done in one line to avoid building helper.plugins multiple times, or by accessing the tray_items which is probably cheaper)?

jdaviz/app.py

kecnry · 2024-03-01T13:05:59Z

jdaviz/app.py

@@ -1995,7 +2024,11 @@ def vue_data_item_remove(self, event):

        data_label = event['item_name']
        data = self.data_collection[data_label]
-        self._reparent_subsets(data)
+        if self.config == "imviz":


Can we check something else other than the config? Maybe check if the orientation plugin is loaded (which could probably be done in one line to avoid building helper.plugins multiple times, or by accessing the tray_items which is probably cheaper)?

… angle with flip

rosteen · 2024-03-04T22:41:34Z

@kecnry @pllim Most of the changes today were implementing Kyle's suggestion to check for the Orientation plugin, and fixing a semi-related bug I discovered while working on this (If you created a third orientation when a second one was active, then delete the second one, the third wouldn't be linked to anything). Should be ready for re-review tomorrow, I'll ping you then.

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

rosteen · 2024-03-05T16:02:56Z

@kecnry @pllim This is ready for another look.

pllim

Code looks like code. Some minor comments.

jdaviz/app.py

pllim · 2024-03-05T17:12:33Z

I want to double check that the test catalog failure isn't transient or related. Gonna restart that job.

>       assert catalogs_plugin.number_of_results > 500
E       AssertionError: assert 0 > 500
E        +  where 0 = Catalogs...

jdaviz/configs/imviz/tests/test_catalogs.py:104: AssertionError

pllim · 2024-03-05T18:23:51Z

Nvm I see the same Catalogs failure on main. It is unrelated.

Co-authored-by: P. L. Lim <2090236+pllim@users.noreply.github.com>

pllim · 2024-03-05T20:32:21Z

p.s. FWIW #2738 is to address Catalog failure (that seems to be gone now).

pllim

I pushed my remaining review as a commit, mainly expanding your new test. So this LGTM now. Thanks!

pllim · 2024-03-05T23:43:43Z

jdaviz/configs/imviz/tests/test_orientation.py

@@ -152,3 +156,48 @@ def test_custom_orientation(self):
        lc_plugin._obj.add_orientation(rotation_angle=None, east_left=True, label=None,
                                       set_on_create=True, wrt_data=None)
        assert self.viewer.state.reference_data.label == "CCW 42.00 deg (E-left)"
+
+
+class TestDeleteOrientation(BaseImviz_WCS_WCS):


Note: I made this its own test class so it does not pollute the other tests.

pllim · 2024-03-05T23:44:25Z

jdaviz/configs/imviz/tests/test_orientation.py

+    @pytest.mark.parametrize(
+        ("angle", "sbst_theta"),
+        [(0.5 * u.rad, 2.641593),
+         (-0.5 * u.rad, 3.641589)])


Is -0.5 rad a good input for "negative angle"? If not, feel free to adjust the test.

pllim · 2024-03-05T23:45:23Z

jdaviz/configs/imviz/tests/test_orientation.py

+        lc_plugin._obj.create_north_up_east_left(set_on_create=True)
+
+        # Create rotated shape
+        reg = klass(center=SkyCoord(ra=337.51931488, dec=-20.83187472, unit="deg"),


Providing region as input made more sense to me, so I did that change here. Also added test case for rectangle that seems to have a separate logic route. Luckily ellipse and rectangle have same inputs in regions, so we can easily parametrize them.

pllim · 2024-03-05T23:46:21Z

jdaviz/configs/imviz/tests/test_orientation.py

+        assert_allclose(out_reg.center.dec.deg, reg.center.dec.deg)
+        assert_quantity_allclose(out_reg.width, reg.width)
+        assert_quantity_allclose(out_reg.height, reg.height)
+        # FIXME: However, sky angle has to stay the same as per regions convention.


I think fixing this is out of scope for this PR. If you have a JIRA ticket from the "what does 0 angle mean" meeting today, please link here.

kecnry

Seems to work well! Changing orientation and reparenting definitely isn't fast, so there may be room for optimization either here or elsewhere in the logic, but we can handle that separately. Thanks for the fix!

kecnry · 2024-03-06T13:14:41Z

jdaviz/app.py

+                        if old_flip != new_flip:
+                            relative_angle = 180 - new_angle - old_angle


I think this still assumes some symmetry in the subcomponent of the subset and would break if we ever introduce lasso subsets, right? Maybe just worth a comment so we don't get tripped up by it in the future if we ever add that.

FWIW, there is no lasso shape support in regions nor photutils. So it would probably be turned into some polygon that has no concept of angle anyway (though you would have to rotate it properly or whatever before you turn it into polygon), so maybe also put that in comment?

jdaviz/app.py

rosteen · 2024-03-06T15:22:31Z

Thanks for the test updates @pllim, I added a comment about irregular regions and will merge after tests pass.

rosteen added bug Something isn't working imviz labels Feb 27, 2024

rosteen added this to the 3.9 milestone Feb 27, 2024

pllim reviewed Feb 27, 2024

View reviewed changes

jdaviz/app.py Outdated Show resolved Hide resolved

jdaviz/app.py Outdated Show resolved Hide resolved

pllim reviewed Feb 27, 2024

View reviewed changes

jdaviz/app.py Outdated Show resolved Hide resolved

jdaviz/app.py Outdated Show resolved Hide resolved

rosteen added 5 commits February 28, 2024 09:11

Experimenting...

e575800

Radius calculation now correct under rotation, trying to handle angle…

27ee950

… properly

Account for rotation to update theta attribute

fda63f9

Updated labels since we reparent to Orientation now. Numbers probably…

0ad2da6

… wrong

Added correct calculations for rotated rectangle subset

6ddbb29

rosteen force-pushed the fix-subset-resize branch from 4852d55 to 6ddbb29 Compare February 28, 2024 22:06

Codestyle

66ecdc1

Add to changelog

a11afe5

rosteen marked this pull request as ready for review February 29, 2024 14:46

rosteen requested review from javerbukh, kecnry, bmorris3, haticekaratay, cshanahan1 and gibsongreen as code owners February 29, 2024 14:46