Add ability to retrieve only spectral or spatial subsets

[rosteen]

Fixes a bug when calling cubeviz.specviz.get_spectral_regions() where it would crash if there were circular spatial subsets defined in the image viewer (and would return rectangular spatial subsets like they were spectral regions).

Also prevents rectangular spatial regions from showing up as spectral regions in the Collapse plugin (see screenshot below), and opens up other usage that would require this capability.

Someone more familiar with Glue should look at this as well in case there are any pitfalls with doing the check this way that I don't appreciate. This is the best attribute on the Subsets I could find to determine whether they are spatial or spectral.

[rosteen]

Woah, it looks like my local main branch might be a bit messed up. Whoops.

[codecov]

Codecov Report

Merging #736 (5701cbe) into main (92ce20b) will increase coverage by 0.03%.
The diff coverage is 75.00%.

@@            Coverage Diff             @@
##             main     #736      +/-   ##
==========================================
+ Coverage   62.83%   62.86%   +0.03%     
==========================================
  Files          65       65              
  Lines        4286     4290       +4     
==========================================
+ Hits         2693     2697       +4     
  Misses       1593     1593              

Impacted Files	Coverage Δ
...daviz/configs/default/plugins/collapse/collapse.py	77.45% <0.00%> (ø)
jdaviz/configs/specviz/helper.py	45.36% <0.00%> (ø)
jdaviz/app.py	80.42% <100.00%> (+0.18%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 92ce20b...5701cbe. Read the comment docs.

[pllim]

Interesting solution by adding subset_type. I don't see anything wrong with it, but I also wonder if it is really necessary, or can your logic apply by simply comparing with isinstance.

[pllim]

Also, is it possible to add tests?

[rosteen]

I want this to be available to users without them needing to know what Glue subset attribute to look for to distinguish. It also seemed more elegant to have the isinstance checks centralized in a single location rather than adding them all over the code base.

I'll see about adding a test later today.

[pllim]

New subset_type needs to be documented. Is "spatial" here only for spectroscopy context? What about imagers?

[rosteen]

"spatial" is specifically to get subsets made in image viewers. I just updated the dosctring to (hopefully) make that clearer.

[rosteen]

I added a test for using each option of the subset_type kwarg of the app.get_subsets_from_viewer method. It may be of interest to the other devs particularly to take a look at what I did there, since I think it's a better way to test the subset creation than the existing tests were. Specifically, using the apply_roi method attached to the viewers replicates what actually happens when the viewer ROI selection tools are used, rather than injecting subsets straight into the backend Glue data. I updated the existing profile subset test to do this, but haven't done it for the others yet. (EDIT: I changed those as well)

@pllim I also happened to use the new glue-jupyter functionality you added that resets the subset selection when a new tool is selected (flux_viewer.toolbar.active_tool = flux_viewer.toolbar.tools['bqplot:rectangle']) to let me add multiple subsets in the code.

[orifox]

Confirmed that this works and I approve.

[pllim]

Imviz seems to still work fine with this patch.

I encountered weird behavior after deleting a few interactive regions back to back, but I don't think it is related (probably that lag wrecking havoc on app state).

[duytnguyendtn]

Works as expected, and I appreciate the direction this PR takes subset handling.

As I understand the original intent of subset scope, subsets were originally meant to exist globally outside the context of a particular datatype (i.e. Spatial subsets have a spectral component and vice versa). Unfortunately, users have repeatedly gotten confused between the two to the point I think it may be best to start dividing subsets explicitly into "Spatial" and "Spectral" subsets, as this PR details. E.g. when a spatial subset is created in Cubeviz, a second, spectral, subset would be defined alongside the spatial subset, and an association would be presented to show the spectral subset was autogenerated from the spatial subset. This would be opposed to my understanding of how things happen currently: A spatial subset is drawn, which has a "spectral" definition as well, but are both defined by the same "spatially-defined" subset

[duytnguyendtn]

I do wonder about this approach of creating an roi through a viewer. It matches our UI paradigm (as you can't define an roi without a viewer), but that's more of a context thing. Ideally, through code, you wouldn't need a viewer in order to define a ROI that is supposed to be viewer-independent, right? I'm surprised you can't go even deeper into Glue and define the ROI even lower

[rosteen]

The point was specifically to go less deep into the stack, I wanted to do it in a way that replicates what happens when you use a region selection tool in a viewer.

[rosteen]

For example, this way it matters what the Subset selection state (e.g. "Create new" vs "Subset 1" selected) is rather than injecting the subset deeper with a label. Later in the test I trigger the switch back to "Create new" that Pey Lian added by selecting another tool, so we get to make sure that's working as a bonus.

[pllim]

Pey Lian added by selecting another tool

To be fair... I tried to implement it, but @astrofrog ended up rewriting everything anyway in my PR. So the credit really goes to him... 😅

[rosteen]

Ah, well, nevertheless!