From 8908613f7db7e93aecd038a447e1f1b8ada9a09f Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Tue, 6 May 2025 09:21:28 -0500 Subject: [PATCH 1/8] Fix error in test for load_fits This was not actually testing passing in a file name...it was passing in an HDU. --- src/astro_image_display_api/widget_api_test.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/astro_image_display_api/widget_api_test.py b/src/astro_image_display_api/widget_api_test.py index d515088..351762d 100644 --- a/src/astro_image_display_api/widget_api_test.py +++ b/src/astro_image_display_api/widget_api_test.py @@ -70,9 +70,12 @@ def test_width_height(self): assert self.image.image_width == width assert self.image.image_height == height - def test_load_fits(self, data): + def test_load_fits(self, data, tmp_path): hdu = fits.PrimaryHDU(data=data) - self.image.load_fits(hdu) + image_path = tmp_path / 'test.fits' + hdu.header["BUNIT"] = "adu" + hdu.writeto(image_path) + self.image.load_fits(image_path) def test_load_nddata(self, data): nddata = NDData(data) From 3957d5634182f24aba8a970c83b64275ad6341bb Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Tue, 6 May 2025 16:01:16 -0500 Subject: [PATCH 2/8] Change type of error check --- src/astro_image_display_api/widget_api_test.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/astro_image_display_api/widget_api_test.py b/src/astro_image_display_api/widget_api_test.py index 351762d..b51c1f9 100644 --- a/src/astro_image_display_api/widget_api_test.py +++ b/src/astro_image_display_api/widget_api_test.py @@ -99,11 +99,11 @@ def test_offset_by(self, data, wcs): self.image.offset_by(10 * u.arcmin, 10 * u.arcmin) # A mix of pixel and sky should produce an error - with pytest.raises(ValueError, match='but dy is of type'): + with pytest.raises(u.UnitConversionError, match='are not convertible'): self.image.offset_by(10 * u.arcmin, 10) # A mix of inconsistent units should produce an error - with pytest.raises(u.UnitConversionError): + with pytest.raises(u.UnitConversionError, match='are not convertible'): self.image.offset_by(1 * u.arcsec, 1 * u.AA) def test_zoom_level(self, data): From 2895e82d0e642d61f4617578627a11f100468f37 Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Tue, 6 May 2025 16:02:46 -0500 Subject: [PATCH 3/8] Rename helper method to be more explicit --- .../widget_api_test.py | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/src/astro_image_display_api/widget_api_test.py b/src/astro_image_display_api/widget_api_test.py index b51c1f9..27b4b56 100644 --- a/src/astro_image_display_api/widget_api_test.py +++ b/src/astro_image_display_api/widget_api_test.py @@ -49,7 +49,7 @@ class variable does the trick. """ self.image = self.image_widget_class(image_width=250, image_height=100) - def _check_marker_table_return_properties(self, table): + def _check_empty_marker_table_return_properties(self, table): assert isinstance(table, Table) assert len(table) == 0 assert sorted(table.colnames) == sorted(['x', 'y', 'coord', 'marker name']) @@ -119,7 +119,7 @@ def test_zoom(self): def test_marking_operations(self): marks = self.image.get_markers(marker_name="all") - self._check_marker_table_return_properties(marks) + self._check_empty_marker_table_return_properties(marks) assert not self.image.is_marking # Ensure you cannot set it like this. @@ -160,7 +160,7 @@ def test_marking_operations(self): warnings.simplefilter("error") t = self.image.get_markers(marker_name='markymark') - self._check_marker_table_return_properties(t) + self._check_empty_marker_table_return_properties(t) self.image.click_drag = True self.image.start_marking() @@ -177,7 +177,7 @@ def test_marking_operations(self): self.image.stop_marking(clear_markers=True) assert self.image.is_marking is False - self._check_marker_table_return_properties(self.image.get_markers(marker_name="all")) + self._check_empty_marker_table_return_properties(self.image.get_markers(marker_name="all")) # Hate this, should add to public API marknames = self.image._marktags @@ -250,15 +250,15 @@ def test_add_markers(self): self.image.reset_markers() marknames = self.image._marktags assert len(marknames) == 0 - self._check_marker_table_return_properties(self.image.get_markers(marker_name="all")) + self._check_empty_marker_table_return_properties(self.image.get_markers(marker_name="all")) # Check that no markers remain after clearing - tab = self.image.get_markers(marker_name=self.image._default_mark_tag_name) - self._check_marker_table_return_properties(tab) + tab = self.image.get_markers(marker_name=self.image.DEFAULT_MARKER_NAME) + self._check_empty_marker_table_return_properties(tab) # Check that retrieving a marker set that doesn't exist returns # an empty table with the right columns tab = self.image.get_markers(marker_name='test1') - self._check_marker_table_return_properties(tab) + self._check_empty_marker_table_return_properties(tab) def test_get_markers_accepts_list_of_names(self): # Check that the get_markers method accepts a list of marker names @@ -288,7 +288,7 @@ def test_remove_markers_name_all(self): self.image.add_markers(tab, marker_name='test2') self.image.remove_markers(marker_name='all') - self._check_marker_table_return_properties(self.image.get_markers(marker_name='all')) + self._check_empty_marker_table_return_properties(self.image.get_markers(marker_name='all')) def test_remove_marker_accepts_list(self): data = np.arange(10).reshape(5, 2) @@ -298,7 +298,7 @@ def test_remove_marker_accepts_list(self): self.image.remove_markers(marker_name=['test1', 'test2']) marks = self.image.get_markers(marker_name='all') - self._check_marker_table_return_properties(marks) + self._check_empty_marker_table_return_properties(marks) def test_adding_markers_as_world(self, data, wcs): ndd = NDData(data=data, wcs=wcs) From f89c82c718a0772d07fcf31c1f1f8bc898c9f6ae Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Tue, 6 May 2025 16:04:31 -0500 Subject: [PATCH 4/8] Add helper method for getting set of marker names --- src/astro_image_display_api/widget_api_test.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/astro_image_display_api/widget_api_test.py b/src/astro_image_display_api/widget_api_test.py index 27b4b56..fabe155 100644 --- a/src/astro_image_display_api/widget_api_test.py +++ b/src/astro_image_display_api/widget_api_test.py @@ -54,6 +54,14 @@ def _check_empty_marker_table_return_properties(self, table): assert len(table) == 0 assert sorted(table.colnames) == sorted(['x', 'y', 'coord', 'marker name']) + def _get_marker_names_as_set(self): + marks = self.image.get_markers(marker_name="all")["marker name"] + if hasattr(marks, 'mask') and all(marks.mask): + marker_names = set() + else: + marker_names = set(marks) + return marker_names + def test_default_marker_names(self): # Check only that default names are set to a non-empty string assert self.image.DEFAULT_MARKER_NAME From 57a04dc5bc1f1cfd2a513a96c253260228ca5089 Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Tue, 6 May 2025 16:05:51 -0500 Subject: [PATCH 5/8] Only use public API in the API tests Which, admittedly, should have been the approach from the beginning. --- .../widget_api_test.py | 43 +++++++++---------- 1 file changed, 21 insertions(+), 22 deletions(-) diff --git a/src/astro_image_display_api/widget_api_test.py b/src/astro_image_display_api/widget_api_test.py index fabe155..97c03cc 100644 --- a/src/astro_image_display_api/widget_api_test.py +++ b/src/astro_image_display_api/widget_api_test.py @@ -174,12 +174,12 @@ def test_marking_operations(self): self.image.start_marking() assert not self.image.click_drag - # Simulate a mouse click to add default marker name to the list. - try: - self.image._mouse_click_cb(self.image.viewer, None, 50, 50) - assert self.image.get_marker_names() == [self.image._interactive_marker_set_name, 'markymark'] - except AttributeError: - pass + # Add a marker to the interactive marking table + self.image.add_markers( + Table(data=[[50], [50]], names=['x', 'y'], dtype=('float', 'float')), + marker_name=self.image.DEFAULT_INTERACTIVE_MARKER_NAME, + ) + assert self._get_marker_names_as_set() == set([self.image.DEFAULT_INTERACTIVE_MARKER_NAME]) # Clear markers to not pollute other tests. self.image.stop_marking(clear_markers=True) @@ -188,13 +188,14 @@ def test_marking_operations(self): self._check_empty_marker_table_return_properties(self.image.get_markers(marker_name="all")) # Hate this, should add to public API - marknames = self.image._marktags + marknames = self._get_marker_names_as_set() assert len(marknames) == 0 # Make sure that click_drag is restored as expected assert self.image.click_drag def test_add_markers(self): + original_marker_name = self.image.DEFAULT_MARKER_NAME data = np.arange(10).reshape(5, 2) orig_tab = Table(data=data, names=['x', 'y'], dtype=('float', 'float')) tab = Table(data=data, names=['x', 'y'], dtype=('float', 'float')) @@ -202,7 +203,7 @@ def test_add_markers(self): skycoord_colname='coord', marker_name='test1') # Make sure setting didn't change the default name - assert self.image._default_mark_tag_name == 'default-marker-name' + assert self.image.DEFAULT_MARKER_NAME == original_marker_name # Regression test for GitHub Issue 45: # Adding markers should not modify the input data table. @@ -212,7 +213,7 @@ def test_add_markers(self): self.image.add_markers(tab, x_colname='x', y_colname='y', skycoord_colname='coord', marker_name='test2') - marknames = self.image._marktags + marknames = self._get_marker_names_as_set() assert marknames == set(['test1', 'test2']) # assert self.image.get_marker_names() == ['test1', 'test2'] @@ -234,7 +235,7 @@ def test_add_markers(self): assert (t2['y'] == expected['y']).all() self.image.remove_markers(marker_name='test1') - marknames = self.image._marktags + marknames = self._get_marker_names_as_set() assert marknames == set(['test2']) # assert self.image.get_marker_names() == ['test2'] @@ -249,14 +250,12 @@ def test_add_markers(self): skycoord_colname='coord') # Don't care about the order of the marker names so use set instead of # list. - marknames = self.image._marktags - assert (set(marknames) == set(['test2', self.image._default_mark_tag_name])) - # assert (set(self.image.get_marker_names()) == - # set(['test2', self.image._default_mark_tag_name])) + marknames = self._get_marker_names_as_set() + assert (set(marknames) == set(['test2', self.image.DEFAULT_MARKER_NAME])) # Clear markers to not pollute other tests. self.image.reset_markers() - marknames = self.image._marktags + marknames = self._get_marker_names_as_set() assert len(marknames) == 0 self._check_empty_marker_table_return_properties(self.image.get_markers(marker_name="all")) # Check that no markers remain after clearing @@ -391,10 +390,10 @@ def test_click_drag(self): # If is_marking is true then trying to enable click_drag should fail self.image.click_drag = False - self.image._is_marking = True - with pytest.raises(ValueError, match=r'([Ii]nteractive marking)|(while in marking mode)'): + self.image.start_marking() + with pytest.raises(ValueError, match=r'([Ii]nteractive marking)|(while in marking mode)|(while marking is active)'): self.image.click_drag = True - self.image._is_marking = False + self.image.stop_marking() def test_click_center(self): # Set this to ensure that click_center turns it off @@ -408,12 +407,12 @@ def test_click_center(self): self.image.click_center = True assert not self.image.click_drag - # If is_marking is true then trying to enable click_center should fail - self.image._is_marking = True self.image.click_center = False - with pytest.raises(ValueError, match=r'([Ii]nteractive marking)|(while in marking mode)'): + # If is_marking is true then trying to enable click_center should fail + self.image.start_marking() + with pytest.raises(ValueError, match=r'([Ii]nteractive marking)|(while marking is active)'): self.image.click_center = True - self.image._is_marking = False + self.image.stop_marking() def test_scroll_pan(self): # Make sure scroll_pan is actually settable From 9a04a5b2b12e84cc17ed490a2537eee90a5e9884 Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Tue, 6 May 2025 16:06:14 -0500 Subject: [PATCH 6/8] Fix minor errors in test logic --- src/astro_image_display_api/widget_api_test.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/astro_image_display_api/widget_api_test.py b/src/astro_image_display_api/widget_api_test.py index 97c03cc..fd7de5e 100644 --- a/src/astro_image_display_api/widget_api_test.py +++ b/src/astro_image_display_api/widget_api_test.py @@ -142,6 +142,7 @@ def test_marking_operations(self): # Set the marker style marker_style = {'color': 'yellow', 'radius': 10, 'type': 'cross'} + self.image.marker = marker_style m_str = str(self.image.marker) for key in marker_style.keys(): assert key in m_str @@ -314,14 +315,13 @@ def test_adding_markers_as_world(self, data, wcs): # Add markers using world coordinates pixels = np.linspace(0, 100, num=10).reshape(5, 2) marks_pix = Table(data=pixels, names=['x', 'y'], dtype=('float', 'float')) - marks_world = wcs.pixel_to_world(marks_pix['x'], marks_pix['y']) - marks_coords = SkyCoord(marks_world, unit='degree') + marks_coords = wcs.pixel_to_world(marks_pix['x'], marks_pix['y']) mark_coord_table = Table(data=[marks_coords], names=['coord']) self.image.add_markers(mark_coord_table, use_skycoord=True) result = self.image.get_markers() # Check the x, y positions as long as we are testing things... # The first test had one entry that was zero, so any check - # based on rtol will will. Added a small atol to make sure + # based on rtol will not work. Added a small atol to make sure # the test passes. np.testing.assert_allclose(result['x'], marks_pix['x'], atol=1e-9) np.testing.assert_allclose(result['y'], marks_pix['y']) @@ -336,7 +336,7 @@ def test_stretch(self): original_stretch = self.image.stretch - with pytest.raises(ValueError, match='must be one of'): + with pytest.raises(ValueError, match='[mM]ust be one of'): self.image.stretch = 'not a valid value' # A bad value should leave the stretch unchanged @@ -347,7 +347,7 @@ def test_stretch(self): assert self.image.stretch is not original_stretch def test_cuts(self, data): - with pytest.raises(ValueError, match='must be one of'): + with pytest.raises(ValueError, match='[mM]ust be one of'): self.image.cuts = 'not a valid value' with pytest.raises(ValueError, match='must have length 2'): From 804af6002b336bfa119003720fba88f8c889da33 Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Tue, 6 May 2025 16:06:40 -0500 Subject: [PATCH 7/8] Mark colormap tests as xfail It is not even clear colormaps are part of the API... --- src/astro_image_display_api/widget_api_test.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/astro_image_display_api/widget_api_test.py b/src/astro_image_display_api/widget_api_test.py index fd7de5e..165dbe2 100644 --- a/src/astro_image_display_api/widget_api_test.py +++ b/src/astro_image_display_api/widget_api_test.py @@ -363,6 +363,7 @@ def test_cuts(self, data): self.image.cuts = (10, 100) assert self.image.cuts == (10, 100) + @pytest.mark.xfail(reason="Not clear whether colormap is part of the API") def test_colormap(self): cmap_desired = 'gray' cmap_list = self.image.colormap_options From b01d937ead7805a53c6652d43fc656e8ab1a767c Mon Sep 17 00:00:00 2001 From: Matt Craig Date: Tue, 6 May 2025 16:14:59 -0500 Subject: [PATCH 8/8] Rename method so copilot likes it --- src/astro_image_display_api/widget_api_test.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/astro_image_display_api/widget_api_test.py b/src/astro_image_display_api/widget_api_test.py index 165dbe2..9bec5e6 100644 --- a/src/astro_image_display_api/widget_api_test.py +++ b/src/astro_image_display_api/widget_api_test.py @@ -49,7 +49,7 @@ class variable does the trick. """ self.image = self.image_widget_class(image_width=250, image_height=100) - def _check_empty_marker_table_return_properties(self, table): + def _assert_empty_marker_table(self, table): assert isinstance(table, Table) assert len(table) == 0 assert sorted(table.colnames) == sorted(['x', 'y', 'coord', 'marker name']) @@ -127,7 +127,7 @@ def test_zoom(self): def test_marking_operations(self): marks = self.image.get_markers(marker_name="all") - self._check_empty_marker_table_return_properties(marks) + self._assert_empty_marker_table(marks) assert not self.image.is_marking # Ensure you cannot set it like this. @@ -169,7 +169,7 @@ def test_marking_operations(self): warnings.simplefilter("error") t = self.image.get_markers(marker_name='markymark') - self._check_empty_marker_table_return_properties(t) + self._assert_empty_marker_table(t) self.image.click_drag = True self.image.start_marking() @@ -186,7 +186,7 @@ def test_marking_operations(self): self.image.stop_marking(clear_markers=True) assert self.image.is_marking is False - self._check_empty_marker_table_return_properties(self.image.get_markers(marker_name="all")) + self._assert_empty_marker_table(self.image.get_markers(marker_name="all")) # Hate this, should add to public API marknames = self._get_marker_names_as_set() @@ -258,15 +258,15 @@ def test_add_markers(self): self.image.reset_markers() marknames = self._get_marker_names_as_set() assert len(marknames) == 0 - self._check_empty_marker_table_return_properties(self.image.get_markers(marker_name="all")) + self._assert_empty_marker_table(self.image.get_markers(marker_name="all")) # Check that no markers remain after clearing tab = self.image.get_markers(marker_name=self.image.DEFAULT_MARKER_NAME) - self._check_empty_marker_table_return_properties(tab) + self._assert_empty_marker_table(tab) # Check that retrieving a marker set that doesn't exist returns # an empty table with the right columns tab = self.image.get_markers(marker_name='test1') - self._check_empty_marker_table_return_properties(tab) + self._assert_empty_marker_table(tab) def test_get_markers_accepts_list_of_names(self): # Check that the get_markers method accepts a list of marker names @@ -296,7 +296,7 @@ def test_remove_markers_name_all(self): self.image.add_markers(tab, marker_name='test2') self.image.remove_markers(marker_name='all') - self._check_empty_marker_table_return_properties(self.image.get_markers(marker_name='all')) + self._assert_empty_marker_table(self.image.get_markers(marker_name='all')) def test_remove_marker_accepts_list(self): data = np.arange(10).reshape(5, 2) @@ -306,7 +306,7 @@ def test_remove_marker_accepts_list(self): self.image.remove_markers(marker_name=['test1', 'test2']) marks = self.image.get_markers(marker_name='all') - self._check_empty_marker_table_return_properties(marks) + self._assert_empty_marker_table(marks) def test_adding_markers_as_world(self, data, wcs): ndd = NDData(data=data, wcs=wcs)