diff --git a/CHANGES.rst b/CHANGES.rst index 223531c779..d6c556e400 100644 --- a/CHANGES.rst +++ b/CHANGES.rst @@ -11,7 +11,7 @@ New Features - Opacity for spatial subsets is now adjustable from within Plot Options. [#2663] -- Live-preview of aperture selection in plugins. [#2664] +- Live-preview of aperture selection in plugins. [#2664, #2684] Cubeviz ^^^^^^^ @@ -91,6 +91,8 @@ Cubeviz Imviz ^^^^^ +- Apertures that are selected and later modified to be invalid properly show a warning. [#2684] + Mosviz ^^^^^^ diff --git a/jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py b/jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py index 1c67e03434..26ee9b83ae 100644 --- a/jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py +++ b/jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py @@ -9,7 +9,7 @@ from astropy.nddata import ( NDDataArray, StdDevUncertainty, NDUncertainty ) -from traitlets import Any, Bool, Float, List, Unicode, observe +from traitlets import Any, Bool, Dict, Float, List, Unicode, observe from jdaviz.core.custom_traitlets import FloatHandleEmpty from jdaviz.core.events import SnackbarMessage, SliceWavelengthUpdatedMessage @@ -65,6 +65,7 @@ class SpectralExtraction(PluginTemplateMixin, ApertureSubsetSelectMixin, bg_items = List([]).tag(sync=True) bg_selected = Any('').tag(sync=True) + bg_selected_validity = Dict().tag(sync=True) bg_scale_factor = Float(1).tag(sync=True) bg_wavelength_dependent = Bool(False).tag(sync=True) @@ -100,6 +101,7 @@ def __init__(self, *args, **kwargs): self.background = ApertureSubsetSelect(self, 'bg_items', 'bg_selected', + 'bg_selected_validity', 'bg_scale_factor', dataset='dataset', multiselect=None, diff --git a/jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.vue b/jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.vue index e80426f1b4..d807663f3d 100644 --- a/jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.vue +++ b/jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.vue @@ -19,7 +19,13 @@ hint="Select a spatial region to extract its spectrum." /> -
+ + + {{aperture_selected}} does not support wavelength dependence (cone support): {{aperture_selected_validity.aperture_message}}. + + + +
-
+ + + {{bg_selected}} does not support wavelength dependence (cone support): {{bg_selected_validity.aperture_message}}. + + + +
Extract -
+ + + + Aperture: {{aperture_selected}} does not support subpixel: {{aperture_selected_validity.aperture_message}}. + + + + + Background: {{bg_selected}} does not support subpixel: {{bg_selected_validity.aperture_message}}. + + + + +
diff --git a/jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.py b/jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.py index c21667cd05..5654dac582 100644 --- a/jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.py +++ b/jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.py @@ -286,6 +286,9 @@ def _aperture_selected_changed(self, event={}): if self.multiselect: self._background_selected_changed() return + # NOTE: aperture_selected can be triggered here before aperture_selected_validity is updated + # so we'll still allow the snackbar to be raised as a second warning to the user and to + # avoid acting on outdated information # NOTE: aperture area is only used to determine if a warning should be shown in the UI # and so does not need to be calculated within user API calls that don't act on traitlets @@ -399,13 +402,21 @@ def calculate_photometry(self, dataset=None, aperture=None, background=None, # we can use the pre-cached value data = self.dataset.selected_dc_item - if aperture is not None and aperture not in self.aperture.choices: - raise ValueError(f"aperture must be one of {self.aperture.choices}") + if aperture is not None: + if aperture not in self.aperture.choices: + raise ValueError(f"aperture must be one of {self.aperture.choices}") + if aperture is not None or dataset is not None: reg = self.aperture._get_spatial_region(subset=aperture if aperture is not None else self.aperture.selected, # noqa dataset=dataset if dataset is not None else self.dataset.selected) # noqa + # determine if a valid aperture (since selected_validity only applies to selected entry) + _, _, validity = self.aperture._get_mark_coords_and_validate(selected=aperture) + if not validity.get('is_aperture'): + raise ValueError(f"Selected aperture {aperture} is not valid: {validity.get('aperture_message')}") # noqa else: # use the pre-cached value + if not self.aperture.selected_validity.get('is_aperture'): + raise ValueError(f"Selected aperture is not valid: {self.aperture.selected_validity.get('aperture_message')}") # noqa reg = self.aperture.selected_spatial_region # Reset last fitted model diff --git a/jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.vue b/jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.vue index 7d1185a63f..c38e063baf 100644 --- a/jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.vue +++ b/jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.vue @@ -56,6 +56,12 @@ hint="Select aperture region for photometry (cannot be an annulus or composite subset)." /> + + + {{aperture_selected}} is not a valid aperture: {{aperture_selected_validity.aperture_message}}. + + +
Calculate diff --git a/jdaviz/configs/imviz/tests/test_parser.py b/jdaviz/configs/imviz/tests/test_parser.py index 8cf134ef38..909b42d9df 100644 --- a/jdaviz/configs/imviz/tests/test_parser.py +++ b/jdaviz/configs/imviz/tests/test_parser.py @@ -266,8 +266,10 @@ def test_parse_jwst_nircam_level2(self, imviz_helper): phot_plugin = imviz_helper.app.get_tray_item_from_name('imviz-aper-phot-simple') phot_plugin.data_selected = 'contents[DATA]' phot_plugin.aperture_selected = 'Subset 1' + assert phot_plugin.aperture.selected_validity.get('is_aperture') assert_allclose(phot_plugin.background_value, 0) phot_plugin.background_selected = 'Subset 2' + assert phot_plugin.aperture.selected_validity.get('is_aperture') assert_allclose(phot_plugin.background_value, 0.1741226315498352) # Subset 2 median # NOTE: jwst.datamodels.find_fits_keyword("PHOTMJSR") phot_plugin.counts_factor = (data.meta['photometry']['conversion_megajanskys'] / @@ -396,6 +398,7 @@ def test_parse_hst_drz(self, imviz_helper): phot_plugin = imviz_helper.app.get_tray_item_from_name('imviz-aper-phot-simple') phot_plugin.data_selected = 'contents[SCI,1]' phot_plugin.aperture_selected = 'Subset 1' + assert phot_plugin.aperture.selected_validity.get('is_aperture') phot_plugin.background_value = 0.0014 # Manual entry: Median on whole array assert_allclose(phot_plugin.pixel_area, 0.0025) # Not used but still auto-populated phot_plugin.vue_do_aper_phot() diff --git a/jdaviz/configs/imviz/tests/test_simple_aper_phot.py b/jdaviz/configs/imviz/tests/test_simple_aper_phot.py index 1dd75ebdfc..598ad733db 100644 --- a/jdaviz/configs/imviz/tests/test_simple_aper_phot.py +++ b/jdaviz/configs/imviz/tests/test_simple_aper_phot.py @@ -363,11 +363,18 @@ def test_annulus_background(imviz_helper): PixCoord(x=20.5, y=37.5), inner_radius=20.5, outer_radius=30.5) imviz_helper.load_regions([ellipse_1, annulus_2]) - # Subset 4 (annulus) should be available for the background but not the aperture - assert 'Subset 4' not in phot_plugin.aperture.choices + # Subset 4 (annulus) should be available in both sets of choices, but invalid for selection as + # aperture + assert 'Subset 4' in phot_plugin.aperture.choices assert 'Subset 4' in phot_plugin.background.choices + phot_plugin.aperture_selected = 'Subset 4' + assert not phot_plugin.aperture.selected_validity.get('is_aperture', True) + with pytest.raises(ValueError, match="Selected aperture is not valid"): + phot_plugin.calculate_photometry() + phot_plugin.aperture_selected = 'Subset 3' + assert phot_plugin.aperture.selected_validity.get('is_aperture', False) phot_plugin.background_selected = 'Subset 4' # Check new annulus for four_gaussians diff --git a/jdaviz/core/template_mixin.py b/jdaviz/core/template_mixin.py index 747c59d2c4..545115c96f 100644 --- a/jdaviz/core/template_mixin.py +++ b/jdaviz/core/template_mixin.py @@ -31,7 +31,7 @@ from regions import PixelRegion from specutils import Spectrum1D from specutils.manipulation import extract_region -from traitlets import Any, Bool, Float, HasTraits, List, Unicode, observe +from traitlets import Any, Bool, Dict, Float, HasTraits, List, Unicode, observe from ipywidgets import widget_serialization from ipypopout import PopoutButton @@ -1972,6 +1972,7 @@ class ApertureSubsetSelect(SubsetSelect): * ``items`` (list of dicts with keys: label, color, type) * ``selected`` (string) + * ``selected_validity`` (dict) Properties (in the object only): @@ -2005,7 +2006,8 @@ class ApertureSubsetSelect(SubsetSelect): /> """ - def __init__(self, plugin, items, selected, scale_factor, multiselect=None, + def __init__(self, plugin, items, selected, selected_validity, + scale_factor, multiselect=None, dataset=None, viewers=None, default_text=None): """ Parameters @@ -2016,6 +2018,8 @@ def __init__(self, plugin, items, selected, scale_factor, multiselect=None, the name of the items traitlet defined in ``plugin`` selected : str the name of the selected traitlet defined in ``plugin`` + selected_validity: str + the name of the selected validity dict traitlet defined in ``plugin`` scale_factor : str the name of the traitlet defining the radius factor for the drawn aperture multiselect : str @@ -2027,17 +2031,18 @@ def __init__(self, plugin, items, selected, scale_factor, multiselect=None, the reference names or ids of the viewer to extract the subregion. If not provided or None, will loop through all references. """ - # NOTE: is_not_composite is assumed in _get_mark_coords + # NOTE: is_not_composite is assumed in _get_mark_coords_and_validate super().__init__(plugin, items=items, selected=selected, multiselect=multiselect, - filters=['is_spatial', 'is_not_composite', 'is_not_annulus'], + filters=['is_spatial'], dataset=dataset, viewers=viewers, default_text=default_text) - self.add_traitlets(scale_factor=scale_factor) + self.add_traitlets(selected_validity=selected_validity, + scale_factor=scale_factor) self.add_observe('is_active', self._plugin_active_changed) self.add_observe(selected, self._update_mark_coords) @@ -2081,7 +2086,7 @@ def marks(self): all_aperture_marks += matches continue - x_coords, y_coords = self._get_mark_coords(viewer) + x_coords, y_coords, self.selected_validity = self._get_mark_coords_and_validate(viewer) mark = ApertureMark( viewer, @@ -2095,17 +2100,42 @@ def marks(self): viewer.figure.marks = viewer.figure.marks + [mark] return all_aperture_marks - def _get_mark_coords(self, viewer): - if not len(self.selected) or not len(self.dataset.selected): - return [], [] - if self.selected in self._manual_options: - return [], [] + def _get_mark_coords_and_validate(self, viewer=None, selected=None): + multiselect = getattr(self, 'multiselect', False) - if getattr(self, 'multiselect', False): + if viewer is None: + viewer = self.app._jdaviz_helper.default_viewer._obj + if selected is None: + selected = self.selected + objs = self.selected_obj if multiselect else [self.selected_obj] + else: + objs = self._get_selected_obj(selected) + if isinstance(selected, str): + selected = [selected] + objs = [objs] + + if not len(selected) or not len(self.dataset.selected): + validity = {'is_aperture': False, + 'aperture_message': 'no subset selected'} + return [], [], validity + if selected in self._manual_options: + validity = {'is_aperture': False, + 'aperture_message': 'no subset selected'} + return [], [], validity + + # if any of the selected entries are composite, then _get_spatial_region + # (or selected_spatial_region) will fail. + if np.any([len(obj) > 1 for obj in objs]): + validity = {'is_aperture': False, + 'aperture_message': 'composite subsets are not supported', + 'is_composite': True} + return [], [], validity + + if multiselect or selected != self.selected: # assume first dataset (for retrieving the region object) # but iterate over all subsets spatial_regions = [self._get_spatial_region(dataset=self.dataset.selected[0], subset=subset) # noqa - for subset in self.selected if subset != self._manual_options] + for subset in selected if subset != self._manual_options] else: # use cached version spatial_regions = [self.selected_spatial_region] @@ -2120,18 +2150,22 @@ def _get_mark_coords(self, viewer): else: wcs = getattr(viewer.state.reference_data, 'coords', None) if wcs is None: - return [], [] + validity = {'is_aperture': False, + 'aperture_message': 'invalid wcs'} + return [], [], validity pixel_region = spatial_region.to_pixel(wcs) roi = regions2roi(pixel_region) # NOTE: this assumes that we'll apply the same radius factor to all subsets (all will # be defined at the same slice for cones in cubes) +# if self.scale_factor == 1.0: # this would catch annulus, which might cause confusion +# pass if hasattr(roi, 'radius'): roi.radius *= self.scale_factor elif hasattr(roi, 'radius_x'): roi.radius_x *= self.scale_factor roi.radius_y *= self.scale_factor - elif hasattr(roi, 'center'): + elif hasattr(roi, 'center') and hasattr(roi, 'xmin') and hasattr(roi, 'xmax'): center = roi.center() half_width = abs(roi.xmax - roi.xmin) * 0.5 * self.scale_factor half_height = abs(roi.ymax - roi.ymin) * 0.5 * self.scale_factor @@ -2139,20 +2173,34 @@ def _get_mark_coords(self, viewer): roi.xmax = center[0] + half_width roi.ymin = center[1] - half_height roi.ymax = center[1] + half_height + elif isinstance(roi, CircularAnnulusROI): + validity = {'is_aperture': False, + 'aperture_message': 'annulus is not a supported aperture'} + return [], [], validity else: # pragma: no cover - raise NotImplementedError - - x, y = roi.to_polygon() - - # concatenate with nan between to avoid line connecting separate subsets - x_coords = np.concatenate((x_coords, np.array([np.nan]), x)) - y_coords = np.concatenate((y_coords, np.array([np.nan]), y)) + # known unsupported shapes: annulus + # TODO: specific case for annulus + validity = {'is_aperture': False, + 'aperture_message': 'shape does not support scale factor'} + return [], [], validity + + if hasattr(roi, 'to_polygon'): + x, y = roi.to_polygon() + + # concatenate with nan between to avoid line connecting separate subsets + x_coords = np.concatenate((x_coords, np.array([np.nan]), x)) + y_coords = np.concatenate((y_coords, np.array([np.nan]), y)) + else: + validity = {'is_aperture': False, + 'aperture_message': 'could not convert roi to polygon'} + return [], [], validity - return x_coords, y_coords + validity = {'is_aperture': True} + return x_coords, y_coords, validity def _update_mark_coords(self, *args): for viewer in self.image_viewers: - x_coords, y_coords = self._get_mark_coords(viewer) + x_coords, y_coords, self.selected_validity = self._get_mark_coords_and_validate(viewer) for mark in self.marks: if mark.viewer != viewer: continue @@ -2184,6 +2232,7 @@ class ApertureSubsetSelectMixin(VuetifyTemplate, HubListener): """ aperture_items = List([]).tag(sync=True) aperture_selected = Any('').tag(sync=True) + aperture_selected_validity = Dict().tag(sync=True) aperture_scale_factor = Float(1).tag(sync=True) def __init__(self, *args, **kwargs): @@ -2191,6 +2240,7 @@ def __init__(self, *args, **kwargs): self.aperture = ApertureSubsetSelect(self, 'aperture_items', 'aperture_selected', + 'aperture_selected_validity', 'aperture_scale_factor', dataset='dataset' if hasattr(self, 'dataset') else None, # noqa multiselect='multiselect' if hasattr(self, 'multiselect') else None) # noqa