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

Aperture validate after selection #2684

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion CHANGES.rst
Original file line number Diff line number Diff line change
Expand Up @@ -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
^^^^^^^
Expand Down Expand Up @@ -91,6 +91,8 @@ Cubeviz
Imviz
^^^^^

- Apertures that are selected and later modified to be invalid properly show a warning. [#2684]

Mosviz
^^^^^^

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)

Expand Down Expand Up @@ -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,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,13 @@
hint="Select a spatial region to extract its spectrum."
/>

<div v-if="aperture_selected !== 'Entire Cube' && dev_cone_support">
<v-row v-if="aperture_selected !== 'Entire Cube' && !aperture_selected_validity.is_aperture && dev_cone_support">
<span class="v-messages v-messages__message text--secondary">
{{aperture_selected}} does not support wavelength dependence (cone support): {{aperture_selected_validity.aperture_message}}.
</span>
</v-row>

<div v-if="aperture_selected_validity.is_aperture && dev_cone_support">
<v-row>
<v-switch
v-model="wavelength_dependent"
Expand Down Expand Up @@ -74,7 +80,16 @@
</span>
</v-row>

<div v-if="dev_cone_support && wavelength_dependent">
<v-row v-if="bg_selected !== 'None' && !bg_selected_validity.is_aperture && dev_cone_support">
<span class="v-messages v-messages__message text--secondary">
{{bg_selected}} does not support wavelength dependence (cone support): {{bg_selected_validity.aperture_message}}.
</span>
</v-row>

<div v-if="aperture_selected_validity.is_aperture
&& bg_selected_validity.is_aperture
&& wavelength_dependent
&& dev_cone_support">
<v-row>
<v-switch
v-model="bg_wavelength_dependent"
Expand Down Expand Up @@ -102,7 +117,22 @@

<div @mouseover="() => active_step='ext'">
<j-plugin-section-header :active="active_step==='ext'">Extract</j-plugin-section-header>
<div v-if="dev_subpixel_support">

<v-row v-if="aperture_selected !== 'None' && !aperture_selected_validity.is_aperture && dev_subpixel_support">
<span class="v-messages v-messages__message text--secondary">
Aperture: {{aperture_selected}} does not support subpixel: {{aperture_selected_validity.aperture_message}}.
</span>
</v-row>
<v-row v-if="bg_selected !== 'None' && !bg_selected_validity.is_aperture && dev_subpixel_support">
<span class="v-messages v-messages__message text--secondary">
Background: {{bg_selected}} does not support subpixel: {{bg_selected_validity.aperture_message}}.
</span>
</v-row>


<div v-if="(aperture_selected === 'Entire Cube' || aperture_selected_validity.is_aperture)
&& (bg_selected === 'None' || bg_selected_validity.is_aperture)
&& dev_subpixel_support">
<v-row>
<v-switch
v-model="subpixel"
Expand Down Expand Up @@ -138,6 +168,7 @@
action_label="Extract"
action_tooltip="Run spectral extraction with error and mask propagation"
:action_spinner="spinner"
:action_disabled="aperture_selected === bg_selected"
@click:action="spectral_extraction"
></plugin-add-results>

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,9 @@
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
Expand Down Expand Up @@ -399,13 +402,21 @@
# 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

I cannot tell which is better. But we can also just grab the 3rd result and not bother with magic underscores.

Suggested change
_, _, validity = self.aperture._get_mark_coords_and_validate(selected=aperture)
validity = self.aperture._get_mark_coords_and_validate(selected=aperture)[2]

Copy link
Member Author

Choose a reason for hiding this comment

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

I find the underscores to be more readable myself 🤷‍♂️

if not validity.get('is_aperture'):
raise ValueError(f"Selected aperture {aperture} is not valid: {validity.get('aperture_message')}") # noqa

Check warning on line 415 in jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.py

View check run for this annotation

Codecov / codecov/patch

jdaviz/configs/imviz/plugins/aper_phot_simple/aper_phot_simple.py#L415

Added line #L415 was not covered by tests
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
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,12 @@
hint="Select aperture region for photometry (cannot be an annulus or composite subset)."
/>

<v-row v-if="aperture_selected.length && !aperture_selected_validity.is_aperture">
<span class="v-messages v-messages__message text--secondary" style="color: red !important">
{{aperture_selected}} is not a valid aperture: {{aperture_selected_validity.aperture_message}}.
</span>
</v-row>

<div v-if="aperture_selected.length > 0">
<plugin-subset-select
:items="background_items"
Expand Down Expand Up @@ -175,7 +181,7 @@
:results_isolated_to_plugin="true"
@click="do_aper_phot"
:spinner="spinner"
:disabled="aperture_selected === background_selected"
:disabled="aperture_selected === background_selected || !aperture_selected_validity.is_aperture"
>
Calculate
</plugin-action-button>
Expand Down
3 changes: 3 additions & 0 deletions jdaviz/configs/imviz/tests/test_parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -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'] /
Expand Down Expand Up @@ -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()
Expand Down
11 changes: 9 additions & 2 deletions jdaviz/configs/imviz/tests/test_simple_aper_phot.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading
Loading