Imviz region setter/getter

[pllim]

Fix #622

Notes to self:

• http://docs.glueviz.org/en/latest/python_guide/data_tutorial.html#defining-new-subsets
• https://docs.glueviz.org/en/latest/customizing_guide/customization.html#custom-data-translation

TODO

• Make sure sky regions work
• Add tests (grab examples from example notebook + sky regions with/without WCS)
• Figure out linking issues, see Imviz region setter/getter #630 (comment) , Imviz: Finalize linking logic #667
• Update code when regions 0.5 is released with support for GWCS, see Sky region does not understand GWCS astropy/regions#374 (comment)
• Update code when photutils is released with support for GWCS, see Sky aperture does not understand GWCS astropy/photutils#1219 (comment)
• Get approvals
• Open follow-up issues: Modifying regions, roundtripping, GWCS support...

Proof of concept

• my_aper and my_aper_sky are from photutils aperture objects, from pixel and sky, respectively.
• my_reg and my_reg_sky are from regions objects, from pixel and sky, respectively.
• my_mask is from Numpy boolean array.
• Subset 6 and Subset 7 are from manually drawn shapes.

(I don't know what is the extra small red dot in the left image is. I swear it wasn't there when I took the screenshot!)

Problem 1: Cannot move region (out of scope? yes)

Once they are loaded, they cannot be moved around. However, I also noticed this behavior with manually drawn region, so maybe fixing this is out of scope.

With manually drawn region, you can only move it right after drawn. If you click on a different button and come back to it, you can no longer move it.

Problem 2: When multiple regions exist, get_regions() breaks (fixed)

This is fixed for interactive regions. Because we don't have to roundtrip anymore, this does not affect regions regions.

Older notes

Workflow 1: Run the notebook cell to load my_aper. Run the notebook cell to load my_reg. Run the notebook cell that calls imviz.get_regions().

Workflow 2: Run the notebook cell to load my_aper. Manually draw another region. Run the notebook cell that calls imviz.get_regions().

Traceback would look similar to this:

.../jdaviz/configs/imviz/helper.py in get_regions(self)
     88         """
     89         # TODO: Is there a simpler way to do this?
---> 90         return self.app.get_subsets_from_viewer('viewer-1')
     91 
     92 

.../jdaviz/app.py in get_subsets_from_viewer(self, viewer_reference, data_label)
    495                 #  therefore, if the data is already 2d, simply use as is.
    496                 if value.data.ndim == 2:
--> 497                     region = value.data.get_selection_definition(
    498                         format='astropy-regions')
    499                     regions[key] = region

.../glue/core/data.py in get_selection_definition(self, subset_id, format, **kwargs)
    357                 subset = self.subsets[0]
    358             else:
--> 359                 raise ValueError("Several subsets are present, specify which one to retrieve with subset_id= - valid options are:" + format_choices(self._subset_labels, index=True))
    360         elif isinstance(subset_id, str):
    361             matches = [subset for subset in self.subsets if subset.label == subset_id]

ValueError: Several subsets are present, specify which one to retrieve with subset_id= - valid options are:

* 0 or 'my_aper'
* 1 or 'my_reg'

Problem 3: When only one region is loaded, get_regions() still breaks (won't fix)

Because we dropped the roundtripping requirement, this is no longer an issue. I am reserving "Subset N" label for interactive regions, thus in this way, I can know which one was done interactively, and which programmatically, and get around this problem.

Older notes

Workflow 1: Run the notebook cell to load my_aper. Run the notebook cell that calls imviz.get_regions().

Workflow 2: Run the notebook cell to load my_reg. Run the notebook cell that calls imviz.get_regions().

Question: How do you even translate an arbitrary mask back to regions? Is it possible to attach meta data to Subset for round-tripping? See glue-viz/glue-astronomy#30

Traceback would look similar to this:

.../jdaviz/configs/imviz/helper.py in get_regions(self)
     89         """
     90         # TODO: Is there a simpler way to do this?
---> 91         return self.app.get_subsets_from_viewer('viewer-1')
     92 
     93 

.../jdaviz/app.py in get_subsets_from_viewer(self, viewer_reference, data_label)
    495                 #  therefore, if the data is already 2d, simply use as is.
    496                 if value.data.ndim == 2:
--> 497                     region = value.data.get_selection_definition(
    498                         format='astropy-regions')
    499                     regions[key] = region

.../glue/core/data.py in get_selection_definition(self, subset_id, format, **kwargs)
    371         handler = subset_state_translator.get_handler_for(format)
    372 
--> 373         return handler.to_object(subset, **kwargs)
    374 
    375 

.../glue_astronomy/translators/regions.py in to_object(self, subset)
    159 
    160         else:
--> 161             raise NotImplementedError("Subset states of type {0} are not supported"
    162                                       .format(subset_state.__class__.__name__))

NotImplementedError: Subset states of type MaskSubsetState are not supported

Problem 4: Delete button for regions is broken (fixed)

Seems to work fine now, except for existing bug already reported to #642.

Older notes

Workflow: Load my_aper and my_reg. Manually draw another region. Pick your favorite and click on the associated trashcan button. See the region selection menu acting weird.

In the dev version without this patch, deletion kinda works, except for #642.

[eteq]

My hot-take here:

Problem 1: Cannot move region (out of scope?)

I'd say this should be addressed eventually but doesn't need to be in this PR - i.e., it would be nice to be able to load a region, fiddle with it, and then do something with it, but it is not critical as long as I can just draw a new one by hand over the top of it and get that one instead

Problem 2: When multiple regions exist, get_regions() breaks

I think that's a feature not a bug! More sersiously, I have a hazy memory of debating with someone about this, and if it turns out they convinced me, I think it's OK to have to explicitly name the region.

That said, I would also advocate for the behavior I'm asking for in #622 - that is, the no-argument version gives a dictionary or something with all the regions. That might be a trivial fix somewhere higher up in the helper class heirarchy?

Problem 3: When only one region is loaded, get_regions() still breaks

It would be nice to fix this if possible, but... as per #622 (comment) I think it's not critical that we get a round-trip as long as the interactively drawn apertures still give back a valid region object.

Problem 4: Delete button for regions is broken

Also, "not great but not critical" IMHO, as long as it works for interactive regions.

[eteq]

Oh, and for all of those that I was saying "not critical", I think we should definitely make issues for them and try to fix them, but they don't have to block this PR nor stop closing of #622.

[pllim]

This is to fix a real bug that affects even interactive regions.

[pllim]

@astrofrog , is there any reason not to call data_collection directly? I don't see any unintended consequences but you know better.

[pllim]

Note to self: I wonder if linking problem can be deferred if I use the "reference image" and take away data_label option...

[pllim]

Note for future self: Since Subset is global, I think it is actually loaded 4 times here (twice for each parametrize call). But since the region name is the same, the second parametrize call probably overwrote the first one. But that is probably okay for testing because we just want to check nothing crashed.

[codecov]

Codecov Report

Merging #630 (a906548) into main (1aa0dce) will increase coverage by 0.34%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #630      +/-   ##
==========================================
+ Coverage   59.60%   59.94%   +0.34%     
==========================================
  Files          65       65              
  Lines        4005     4034      +29     
==========================================
+ Hits         2387     2418      +31     
+ Misses       1618     1616       -2     

Impacted Files	Coverage Δ
jdaviz/app.py	78.07% <100.00%> (+0.10%)	⬆️
jdaviz/configs/imviz/helper.py	98.46% <100.00%> (+3.72%)	⬆️
jdaviz/configs/imviz/plugins/parsers.py	97.18% <0.00%> (+0.70%)	⬆️

---

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 1aa0dce...a906548. Read the comment docs.

[pllim]

The linking stuff added in #611 is messing up the regions stuff here. I am not sure how to make the notebook play nice with linked data. Need @astrofrog to chime in here.

The biggest problem is masked subset creation chokes when the second image is too small. I get IndexError.

The smaller problem is when you uncheck/recheck the boxes so linked images show properly for blinking, the second image is displayed. Which means all the cells that execute on layer[0] is not really showing the user anything. The coordinates info bar also acts weird; at one point, it was reporting the info from first image when I am looking at the second image (very confusing!).

Perhaps the linking problem needs to be solved before we can proceed here.

[pllim]

This part of the code needs updating when new release of regions with GWCS support is done, see astropy/regions#374 (comment) .

[pllim]

Too many conflicts now. Work continues on #721.