Enable editing of composite subsets in the subset plugin

[javerbukh]

Description

This pull request is to address editing of composite subsets in the Subset Plugin. Still working on issues related to spectral subsets but updating is at least possible, if not a little inconvenient at the moment. That issue is being tracked here and on slack. It is not introduced by this PR.

XOR functionality has been added to this PR and the ADD mode has been fixed. A previous version did not cover all possible cases and tests have been added to make sure those cases are covered for the future.

Fixes #2162
Fixes #1568

Change log entry

• Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
  list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
  should add a no-changelog-entry-needed label.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

• Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
• Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
• Do the proposed changes follow the STScI Style Guides?
• Are tests added/updated as required? If so, do they follow the STScI Style Guides?
• Are docs added/updated as required? If so, do they follow the STScI Style Guides?
• Did the CI pass? If not, are the failures related?
• Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone.
• After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

[codecov]

Codecov Report

Patch coverage: 93.10% and project coverage change: +0.02 🎉

Comparison is base (1c1fcac) 91.67% compared to head (c4f6f85) 91.69%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2182      +/-   ##
==========================================
+ Coverage   91.67%   91.69%   +0.02%     
==========================================
  Files         148      148              
  Lines       16292    16322      +30     
==========================================
+ Hits        14936    14967      +31     
+ Misses       1356     1355       -1     

Impacted Files	Coverage Δ
...igs/default/plugins/subset_plugin/subset_plugin.py	96.37% <91.66%> (-0.76%)	⬇️
jdaviz/app.py	93.15% <100.00%> (+0.39%)	⬆️
jdaviz/configs/specviz/tests/test_helper.py	100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

[pllim]

(This comment might be out of scope, not sure.)

With glue-viz/glue#2391 (which we cannot use until we take care of #2183), I feel like the recenter, get_center, and set_center stuff can use the unified subset_state.center() to get the center, and subset_state.move_to(...) to set the center. Then, we might be able to simplify a lot of the logic here.

Also, as @dhomeier pointed out, that glue PR upstream deprecated roi.get_center() calls. The deprecation warning will fail our test suite. So either we have to do glue version check or pin to new glue version after #2183 is resolved.

Also, I don't know why Past Self implemented centering logic for shapes that Imviz does not use (range subset). Do you use it? 🤯 🤷

[pllim]

So, to test this behavior with annulus, install the dev version of glue-astronomy, and then you can do something like this (either with the notebook patch I show here or slap that code on your own use case though you'll have to adjust parameter values as you see fit). For some reason, I need to first select the annulus subset from top-left drop-down for the Subset Tool plugin drop-down to update, but since what I am doing is a bit hacky, let's not worry about that for now.

--- a/notebooks/concepts/imviz_simple_aper_phot.ipynb
+++ b/notebooks/concepts/imviz_simple_aper_phot.ipynb
@@ -158,6 +158,22 @@
     "imviz.load_regions([my_aper, my_bg])"
    ]
   },
+  {
+   "cell_type": "code",
+   "execution_count": null,
+   "id": "c6e793a0",
+   "metadata": {},
+   "outputs": [],
+   "source": [
+    "from glue_astronomy.translators.regions import _annulus_to_subset_state\n",
+    "from regions import CircleAnnulusPixelRegion\n",
+    "\n",
+    "data = imviz.app.data_collection[0]\n",
+    "ann_reg = CircleAnnulusPixelRegion(PixCoord(x=1002, y=1154), inner_radius=22, outer_radius=25)\n",
+    "ann_sbst = _annulus_to_subset_state(ann_reg, data)\n",
+    "imviz.app.data_collection.new_subset_group(subset_state=ann_sbst);"
+   ]
+  },
   {

Your edit seems to work for annulus! Though I have to be careful to just modify the radius and the correct one at that.

If I am not careful, I can end up with shape like this, though you get what you ask for, so not technically a bug but might also annoy people if they want to use it for photometry because then they have to manually fix the parameters until it is a "true" circular annulus again.

I can also make the inner radius larger than or equal to the outer radius. Glue does not complain. The subset disappears. And then re-appears when I make the inner radius smaller again.

[javerbukh]

@pllim I would prefer to not mess with get_center, set_center, etc. since - afaik - centering composite subsets is a *checks wiki* gopher or fox hole. Happy to discuss implementation details and get that into a ticket though!

Also, glad to see annulus editing works out of the box!

[pllim]

@javerbukh , then you will have to deal with the deprecation warnings when we unpin maxversion of glue. They are already deprecated.

[camipacifici]

Works very nicely, thank you!
A couple of things I noticed that may or may not be bugs:

• in cubeviz, i select a composite region in the flux viewer, then set the function to 'median', then select a spectral region. the spectral region highlights only the "full cube" collapsed spectrum, but not the collapsed spectrum from the composite spatial region.

• if i do cubeviz.get_interactive_regions() i hit a traceback. (in my notebook cubeviz=h)
  AttributeError Traceback (most recent call last)
  Cell In[4], line 1
  ----> 1 h.get_interactive_regions()

File ~/opt/miniconda3/envs/pr2182/lib/python3.10/site-packages/jdaviz/core/helpers.py:759, in ImageConfigHelper.get_interactive_regions(self)
756 continue
758 try:
--> 759 region = subset_data.data.get_selection_definition(
760 subset_id=subset_label, format='astropy-regions')
761 except (NotImplementedError, ValueError):
762 failed_regs.add(subset_label)

File ~/opt/miniconda3/envs/pr2182/lib/python3.10/site-packages/glue/core/data.py:381, in BaseData.get_selection_definition(self, subset_id, format, **kwargs)
377 subset = self.subsets[subset_id]
379 handler = subset_state_translator.get_handler_for(format)
--> 381 return handler.to_object(subset, **kwargs)

File ~/opt/miniconda3/envs/pr2182/lib/python3.10/site-packages/glue_astronomy/translators/regions.py:175, in AstropyRegionsHandler.to_object(self, subset)
172 return PointPixelRegion(PixCoord(*subset_state.get_xy(data, 1, 0)))
174 elif isinstance(subset_state, AndState):
--> 175 if _is_annulus(subset_state):
176 return CircleAnnulusPixelRegion(
177 center=PixCoord(x=subset_state.state1.roi.xc, y=subset_state.state1.roi.yc),
178 inner_radius=subset_state.state2.state1.roi.radius,
179 outer_radius=subset_state.state1.roi.radius)
180 else:

File ~/opt/miniconda3/envs/pr2182/lib/python3.10/site-packages/glue_astronomy/translators/regions.py:61, in _is_annulus(subset_state)
56 def _is_annulus(subset_state):
57 # subset_state.state1 = outer circle
58 # subset_state.state2 = inner circle
59 # subset_state.state2 is inverted, so we need its state1
60 return ((not isinstance(subset_state.state1, InvertState)) and
---> 61 isinstance(subset_state.state1.roi, CircularROI) and
62 isinstance(subset_state.state2, InvertState) and
63 isinstance(subset_state.state2.state1.roi, CircularROI) and
64 (subset_state.state1.roi.xc == subset_state.state2.state1.roi.xc) and
65 (subset_state.state1.roi.yc == subset_state.state2.state1.roi.yc) and
66 (subset_state.state1.roi.radius > subset_state.state2.state1.roi.radius))

AttributeError: 'OrState' object has no attribute 'roi'

• one of the regions in the composite region is a rectangle. if i set all xmin, xmax, ymin, ymax to zero and hit update a red banner tells me that i cannot do that, no traceback is issued, but the list of regions in the subset tools disappears. only the menu to select the subset and the update button are left.

[pllim]

AttributeError: 'OrState' object has no attribute 'roi'

That is my bad. I added that check in glue-viz/glue-astronomy#90 and didn't consider all the different things that can trigger the check. I fixed the logic in glue-viz/glue-astronomy#92 but if you need this particular check fixed sooner, I can do a more targeted PR to glue-astronomy.

[camipacifici]

No need, thank you for finding the reason.

[kecnry]

Well done - works well and couldn't find a way to break it (yet)! I think all the changes to the tests look reasonable, but an extra set of eyes or two there wouldn't hurt.

I would still love to see live-editing (although I know there isn't a consensus on that yet)... is that definitely out of scope here? Would that need the changes upstream so we can directly edit the state instead of needing to replace it?

This is going to be difficult to work into a user-friendly API, but I don't see any obvious ways of improving that right now, so that can be deferred.

We'll also want to rebase the display units branch on this as soon as its merged to avoid difficulty rebasing #2195.

[kecnry]

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

[javerbukh]

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).

[kecnry]

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?

[javerbukh]

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

[javerbukh]

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.

[pllim]

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.

[javerbukh]

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.

[javerbukh]

@camipacifici I haven't touched get_interactive_regions throughout this refactor since I'm not sure what use case that would have over get_susbets. If get_subsets covers all the use cases, can we deprecate that method?

@pllim Are you working for a fix for that bug or should I add more logic to this PR to avoid that traceback?

[pllim]

I lumped the bug fix into a new feature at glue-viz/glue-astronomy#92 . Do you want me to separate it out?

[javerbukh]

@camipacifici I see the bug you mentioned about the spectral subset not highlighting the collapsed composite spatial subset in the spectrum viewer. I know @kecnry did some work on that functionality a while ago, is it worth trying to get working in this PR or as a follow up?

cubeviz.get_interactive_regions() does not seem to be able to handle composite subsets, so I would suggest using app.get_subsets as a replacement.

Setting all values to 0, or making the upper bound less than the lower bound, or setting a negative radius will all cause bugs/tracebacks. I could spend some time putting guardrails up so that users cannot do some of these things but...why would a user do them? Can they just...not? 😄

[kecnry]

I see the bug you mentioned about the spectral subset not highlighting the collapsed composite spatial subset in the spectrum viewer.

I can reproduce this in main. It works if you create the spectral subset before making the spatial subset a compound subset. I suspect its a limitation of this function in the code... I'll investigate and create an issue if its not a trivial fix. Either way, I think it can be considered out-of-scope for the sake of this PR.

EDIT: see #2207 for an attempt at fixing.

[camipacifici]

Can they just...not?

Well yes, but I ended up in that trap because I wanted to delete part of the compound subset and I thought I could just change it to all zeros for that ;)

[javerbukh]

Unfortunately, there is no way to delete individual subregions in a compound subset. You can go into replace mode and start over if you added a region you did not actually want. If that is a feature users want, it would have to be a separate ticket so we could determine where that can happen and what the UI looks like.

[camipacifici]

Understood. I agree to have the request for this new functionality in a new ticket.
I still think the tool could get out of the problem (like setting all the properties of the subset to zero) a little more gracefully.

[javerbukh]

@camipacifici I agree with that, and I think your creative (😄) way of removing subregions might be a good intermediate step until we can have a more formal way to delete subregions. I can spend some time today seeing if I can add those guardrails in a simple way, I just worry preventing all of those actions (or interpreting them the way the user wants us to) may be a rabbit hole. A more graceful exit should be doable though.

[pllim]

If @cshanahan1 doesn't get to it, I will approve based on my annulus comment above. Worked really well.

[pllim]

But... needs a change log under new feature.

[javerbukh]

Guardrails added and changelog updated, will wait for CI to pass and then I'll merge. Thank you everyone!