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.

Sign up for GitHub

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

Jump to bottom

Enable editing of composite subsets in the subset plugin #2182

Merged
merged 2 commits into from
May 16, 2023
Merged

Enable editing of composite subsets in the subset plugin #2182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
553d717
Allow editing of composite subsets in the subset plugin
javerbukh May 5, 2023
c4f6f85
Remove commented line
javerbukh May 16, 2023
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
2 changes: 2 additions & 0 deletions CHANGES.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,8 @@ New Features

- Vertical (y-range) zoom tool for all spectrum and spectrum-2d viewers. [#2206]

- Allow Subset Plugin to edit composite subsets. [#2182]

Cubeviz
^^^^^^^

Expand Down
79 changes: 50 additions & 29 deletions jdaviz/app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -909,8 +909,6 @@ def get_subsets(self, subset_name=None, spectral_only=False,
# Remove duplicate spectral regions
if is_spectral and isinstance(subset_region, SpectralRegion):
Copy link
Member

Choose a reason for hiding this comment

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

does this still need the second term now that the elif is removed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It does, because subset_region could be a list object and we do not remove duplicate bounds in that case (since there shouldn't be any).

subset_region = self._remove_duplicate_bounds(subset_region)
elif is_spectral:
subset_region = self._remove_duplicate_bounds_in_dict(subset_region)

if spectral_only and is_spectral:
if object_only and not simplify_spectral:
Expand Down Expand Up @@ -942,22 +940,6 @@ def get_subsets(self, subset_name=None, spectral_only=False,
else:
return all_subsets

def _remove_duplicate_bounds_in_dict(self, subset_region):
new_subset_region = []
for elem in subset_region:
if not new_subset_region:
new_subset_region.append(elem)
continue
unique = True
for elem2 in new_subset_region:
if (elem['region'].lower == elem2['region'].lower and
elem['region'].upper == elem2['region'].upper and
elem['glue_state'] == elem2['glue_state']):
unique = False
if unique:
new_subset_region.append(elem)
return new_subset_region

def _is_subset_spectral(self, subset_region):
if isinstance(subset_region, SpectralRegion):
return True
Expand Down Expand Up @@ -1060,23 +1042,34 @@ def get_sub_regions(self, subset_state, simplify_spectral=True):
return new_spec
else:
if isinstance(two, list):
# two[0]['glue_state'] = subset_state.state2.__class__.__name__
two[0]['glue_state'] = "AndNotState"
# Return two first so that we preserve the chronology of how
# subset regions are applied.
return one + two
elif subset_state.op is operator.and_:
# This covers the AND subset mode

# Example of how this works:
# a = SpectralRegion(4 * u.um, 7 * u.um)
# b = SpectralRegion(5 * u.um, 6 * u.um)
# Example of how this works with "one" being the AND region
# and "two" being two Range subsets connected by an OR state:
# one = SpectralRegion(4.5 * u.um, 7.5 * u.um)
# two = SpectralRegion(4 * u.um, 5 * u.um) + SpectralRegion(7 * u.um, 8 * u.um)
#
# b.invert(a.lower, a.upper)
# oppo = two.invert(one.lower, one.upper)
# Spectral Region, 1 sub-regions:
# (5.0 um, 7.0 um)
#
# oppo.invert(one.lower, one.upper)
# Spectral Region, 2 sub-regions:
# (4.0 um, 5.0 um) (6.0 um, 7.0 um)
# (4.5 um, 5.0 um) (7.0 um, 7.5 um)
if isinstance(two, SpectralRegion):
return two.invert(one.lower, one.upper)
# Taking an AND state of an empty region is allowed
# but there is no way for SpectralRegion to display that information.
# Instead, we raise a ValueError
if one.upper.value < two.lower.value or one.lower.value > two.upper.value:
raise ValueError("AND mode should overlap with existing subset")
Comment on lines +1068 to +1069
Copy link
Member

Choose a reason for hiding this comment

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

how would this get triggered? I tried creating this situation in specviz and it displays the state as I'd expect in the plugin and just doesn't highlight anything in the viewer (as expected), but I didn't see any traceback. Is this not needed? Or is the traceback caught somewhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, thought I wrote a test for this one. Let me look at this for a bit and get back to you.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I checked, the problem is: what should be returned from the result of applying Range, then OR, then AND on an empty region (i.e. not where the two subregions are)? Ideally it should be empty but we cannot send an empty SpectralRegion back up the recursive tree. I chose to do this instead since why would someone AND an empty region?

I agree this is an issue but I'm not sure at the moment how to resolve it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there something equivalent to "identity" where it is not empty but it also won't change the result? But I agree it might be out of scope to fix here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When you say "identity", what do you mean? I couldn't find anything that achieved that goal but I agree that should be out of scope for this PR.

oppo = two.invert(one.lower, one.upper)

return oppo.invert(one.lower, one.upper)
else:
return two + one
elif subset_state.op is operator.or_:
Expand All @@ -1089,10 +1082,38 @@ def get_sub_regions(self, subset_state, simplify_spectral=True):
elif two:
return two
elif subset_state.op is operator.xor:
# This covers the XOR case which is currently not working
return None
else:
return None
# This covers the ADD subset mode

# Example of how this works, with "one" acting
# as the XOR region and "two" as two ranges joined
# by an OR:
# a = SpectralRegion(4 * u.um, 5 * u.um)
# b = SpectralRegion(6 * u.um, 9 * u.um)
#
# one = SpectralRegion(4.5 * u.um, 12 * u.um)
# two = a + b

# two.invert(one.lower, one.upper)
# Spectral Region, 2 sub-regions:
# (5.0 um, 6.0 um) (9.0 um, 12.0 um)

# one.invert(two.lower, two.upper)
# Spectral Region, 1 sub-regions:
# (4.0 um, 4.5 um)

# two.invert(one.lower, one.upper) + one.invert(two.lower, two.upper)
# Spectral Region, 3 sub-regions:
# (4.0 um, 4.5 um) (5.0 um, 6.0 um) (9.0 um, 12.0 um)

if isinstance(two, SpectralRegion):
if one.lower > two.lower:
# If one.lower is less than two.lower, it will be included
# in the two.invert() call. Otherwise, we can add it like this.
return (two.invert(one.lower, one.upper) +
one.invert(two.lower, two.upper))
return two.invert(one.lower, one.upper)
else:
return two + one
else:
# This gets triggered in the InvertState case where state1
# is an object and state2 is None
Expand Down
88 changes: 64 additions & 24 deletions jdaviz/configs/default/plugins/subset_plugin/subset_plugin.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ class SubsetPlugin(PluginTemplateMixin, DatasetSelectMixin):

subplugins_opened = Any().tag(sync=True)

is_editable = Bool(False).tag(sync=True)
is_centerable = Bool(False).tag(sync=True)

def __init__(self, *args, **kwargs):
super().__init__(*args, **kwargs)
Expand Down Expand Up @@ -96,7 +96,7 @@ def _sync_selected_from_ui(self, change):
self.subset_definitions = []
self.subset_types = []
self.glue_state_types = []
self.is_editable = False
self.is_centerable = False

if not hasattr(self, 'subset_select'):
# during initial init, this can trigger before the component is initialized
Expand Down Expand Up @@ -132,9 +132,9 @@ def _unpack_get_subsets_for_ui(self):
if not subset_information:
return
if len(subset_information) == 1:
self.is_editable = True
self.is_centerable = True
else:
self.is_editable = False
self.is_centerable = False

for spec in subset_information:
subset_definition = []
Expand Down Expand Up @@ -200,35 +200,75 @@ def _get_subset_definition(self, *args):
self._unpack_get_subsets_for_ui()

def vue_update_subset(self, *args):
status, reason = self._check_input()
if not status:
self.hub.broadcast(SnackbarMessage(reason, color='error', sender=self))
return

for index, sub in enumerate(self.subset_definitions):
if len(self.subset_states) <= index:
return
sub_states = self.subset_states[index]
for d_att in sub:
if d_att["att"] == 'theta': # Humans use degrees but glue uses radians
d_val = np.radians(d_att["value"])
else:
d_val = float(d_att["value"])

if float(d_att["orig"]) != d_val:
if self.subset_types[index] == "Range":
setattr(sub_states, d_att["att"], d_val)
else:
setattr(sub_states.roi, d_att["att"], d_val)
try:
# TODO: This commented out section is "more correct" because it
# adds the changed subregion to the data_collection.subset_groups
# tree. However, it still needs improvement and the section below
# allows updating similar to `main`.
# self.session.edit_subset_mode._combine_data(sub_states,
# override_mode=SUBSET_MODES[self.glue_state_types[index]])
self.session.edit_subset_mode._combine_data(sub_states, override_mode=ReplaceMode)
except Exception as err: # pragma: no cover
self.hub.broadcast(SnackbarMessage(
f"Failed to update Subset: {repr(err)}", color='error', sender=self))
try:
dc = self.data_collection
subsets = dc.subset_groups
self.session.edit_subset_mode._combine_data(
subsets[[x.label for x in subsets].index(self.subset_selected)].subset_state,
override_mode=ReplaceMode)
except Exception as err: # pragma: no cover
self.hub.broadcast(SnackbarMessage(
f"Failed to update Subset: {repr(err)}", color='error', sender=self))

def _check_input(self):
status = True
reason = ""
for index, sub in enumerate(self.subset_definitions):
lo = hi = xmin = xmax = ymin = ymax = None
for d_att in sub:
if d_att["att"] == "lo":
lo = d_att["value"]
elif d_att["att"] == "hi":
hi = d_att["value"]
elif d_att["att"] == "radius" and d_att["value"] <= 0:
status = False
reason = "Failed to update Subset: radius must be a positive scalar"
break
elif d_att["att"] == "xmin":
xmin = d_att["value"]
elif d_att["att"] == "xmax":
xmax = d_att["value"]
elif d_att["att"] == "ymin":
ymin = d_att["value"]
elif d_att["att"] == "ymax":
ymax = d_att["value"]

if lo and hi and hi <= lo:
status = False
reason = "Failed to update Subset: lower bound must be less than upper bound"
break
elif xmin and xmax and ymin and ymax and (xmax - xmin <= 0 or ymax - ymin <= 0):
status = False
reason = "Failed to update Subset: width and length must be positive scalars"
break

return status, reason

def vue_recenter_subset(self, *args):
# Composite region cannot be edited. This only works for Imviz.
if not self.is_editable or self.config != 'imviz': # no-op
# Composite region cannot be centered. This only works for Imviz.
if not self.is_centerable or self.config != 'imviz': # no-op
raise NotImplementedError(
f'Cannot recenter: is_editable={self.is_editable}, config={self.config}')
f'Cannot recenter: is_centerable={self.is_centerable}, config={self.config}')

from photutils.aperture import ApertureStats
from jdaviz.core.region_translators import regions2aperture, _get_region_from_spatial_subset
Expand Down Expand Up @@ -265,16 +305,16 @@ def get_center(self):
cen : number, tuple of numbers, or `None`
The center of the Subset in ``x`` or ``(x, y)``,
depending on the Subset type, if applicable.
If Subset is not editable, this returns `None`.
If Subset is not centerable, this returns `None`.

Raises
------
NotImplementedError
Subset type is not supported.

"""
# Composite region cannot be edited.
if not self.is_editable: # no-op
# Composite region cannot be centered.
if not self.is_centerable: # no-op
return

subset_state = self.subset_select.selected_subset_state
Expand Down Expand Up @@ -303,7 +343,7 @@ def get_center(self):

def set_center(self, new_cen, update=False):
"""Set the desired center for the selected Subset, if applicable.
If Subset is not editable, nothing is done.
If Subset is not centerable, nothing is done.

Parameters
----------
Expand All @@ -322,8 +362,8 @@ def set_center(self, new_cen, update=False):
Subset type is not supported.

"""
# Composite region cannot be edited, so just grab first element.
if not self.is_editable: # no-op
# Composite region cannot be centered, so just grab first element.
if not self.is_centerable: # no-op
return

subset_state = self.subset_select.selected_subset_state
Expand Down
5 changes: 2 additions & 3 deletions jdaviz/configs/default/plugins/subset_plugin/subset_plugin.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
</v-row>

<!-- Sub-plugin for recentering of spatial subset (Imviz only) -->
<v-row v-if="config=='imviz' && is_editable">
<v-row v-if="config=='imviz' && is_centerable">
<v-expansion-panels accordion v-model="subplugins_opened">
<v-expansion-panel>
<v-expansion-panel-header >
Expand Down Expand Up @@ -58,13 +58,12 @@
:label="item.name"
v-model.number="item.value"
type="number"
:disabled="!is_editable"
></v-text-field>
</v-row>
</div>

<v-row justify="end" no-gutters>
<v-btn color="primary" text @click="update_subset" :disabled="!is_editable">Update</v-btn>
<v-btn color="primary" text @click="update_subset">Update</v-btn>
</v-row>
</j-tray-plugin>
</template>
4 changes: 2 additions & 2 deletions jdaviz/configs/specviz/tests/test_helper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ def test_get_spectral_regions_does_not_raise_value_error(self):
def test_get_spectral_regions_composite_region(self):
spectrum_viewer = self.spec_app.app.get_viewer("spectrum-viewer")

self.spec_app.app.get_viewer("spectrum-viewer").apply_roi(XRangeROI(6000, 6400))
self.spec_app.app.get_viewer("spectrum-viewer").apply_roi(XRangeROI(6000, 7400))

spectrum_viewer.session.edit_subset_mode._mode = AndNotMode

Expand All @@ -187,7 +187,7 @@ def test_get_spectral_regions_composite_region(self):
assert_quantity_allclose(spec_region['Subset 1'].subregions[0][0].value,
7300., atol=1e-5)
assert_quantity_allclose(spec_region['Subset 1'].subregions[0][1].value,
7800., atol=1e-5)
7400., atol=1e-5)

def test_get_spectral_regions_composite_region_multiple_and_nots(self):
spectrum_viewer = self.spec_app.app.get_viewer("spectrum-viewer")
Expand Down
Loading