Load DS9 regions into Subsets

[pllim]

Description

This pull request is to allow loading DS9 regions into native Glue subset ROIs instead of the "static regions" that do not work with most plugins.

This is a breaking change for the following APIs:

jdaviz/jdaviz/configs/imviz/helper.py

Line 198 in cbcd65c

def load_static_regions_from_file(self, region_file, region_format='ds9', prefix='region',

jdaviz/jdaviz/configs/imviz/helper.py

Line 236 in cbcd65c

def load_static_regions(self, regions, **kwargs):

jdaviz/jdaviz/configs/imviz/helper.py

Line 343 in cbcd65c

def get_interactive_regions(self):

Pre-requisite for:

• [FEAT] Cubeviz: importing existing region files #1432

Close #1141

TODO

• Add tests for new API.
• Update notebooks (official and concepts).
• Update docs.

Post-merge

• Open follow-up issue to remove the deprecated methods in a future release. See Imviz: Remove deprecated load_static_regions_from_file and load_static_regions #1569
• Open follow-up issue to move region translation code upstream to glue-astronomy. See Move region/Subset translation code to glue-astronomy #1570

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 change log needed? If yes, is it added to CHANGES.rst?
• Is a milestone set?
• After merge, any internal documentations need updating (e.g., JIRA, Innerspace)? 🐱

[codecov]

Codecov Report

Merging #1463 (b9dd2db) into main (13572af) will increase coverage by 0.09%.
The diff coverage is 99.00%.

@@            Coverage Diff             @@
##             main    #1463      +/-   ##
==========================================
+ Coverage   85.52%   85.62%   +0.09%     
==========================================
  Files          94       94              
  Lines        9111     9209      +98     
==========================================
+ Hits         7792     7885      +93     
- Misses       1319     1324       +5     

Impacted Files	Coverage Δ
jdaviz/configs/imviz/helper.py	97.11% <98.75%> (-1.18%)	⬇️
jdaviz/core/region_translators.py	98.85% <100.00%> (+0.34%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[pllim]

This should be ready for review. With this patch, Ori can now load some common regions from DS9 file into Imviz for aperture photometry.

[kecnry]

Just a few questions/suggestions from code-review. I still need to actually test and see how the static vs interactive subsets behave in the various subset/layer dropdowns before approving.

This looks like it deprecates but doesn't actually break existing API, is that right? So even though we don't officially have a semver policy (yet), there wouldn't be any need to bump the major version? Do we need to file a follow-up ticket/reminder to actually remove the deprecated methods?

[kecnry]

I worry that this distinction is too technical for a user. What other subset operations rely on the reference image instead of refdata_label and what are the consquences?

[pllim]

I think it is okay to be a little technical in the API docstring.

I cannot tell you what are the implications because I tried to think of the scenarios and my head started to hurt. It is always murky here because Subsets are global but the linked coordinates transformation depends on what is the reference data in a particular viewer (which can be anything because Imviz is so flexible).

Ideas to make this implementation less confusing while maintaining flexibility is welcome. Otherwise, this is good enough until I get bug reports from Ori?

[kecnry]

does this need to be reverted after the API call to whatever the user had set? Or is there a way to load these regions directly into the state without having to fake them as drawn regions?

[pllim]

does this need to be reverted after the API call to whatever the user had set?

I don't think so? If user is using the API to load a bunch of Subset regions, and then user wants to interactively draw one, chances are user wants to draw a new one.

is there a way to load these regions directly into the state without having to fake them as drawn regions?

apply_roi is the only way I know how to do this. Ideas welcome!

[kecnry]

My only idea would be if we could just add directly to app.data_collection.subset_groups, but I'm not sure if that's writeable or not.

[pllim]

Based on the implementation here, I cannot find a setter for the public property. In theory, you can write to almost anything in Python but since that would involve touching the private attribute, maybe we shouldn't go that route?

https://github.com/glue-viz/glue/blob/578369fa09046471c32fd9c0132961474f678248/glue/core/data_collection.py#L390-L395

[kecnry]

should this belong in AstropyRegionsHandler.to_data then?

[pllim]

Theoretically, yes, but if you look at https://github.com/glue-viz/glue-astronomy/blob/main/glue_astronomy/translators/regions.py at the time of posting:

(a) to_data does not exist at all, so to implement one would include all the shapes that I do not care about as well, which is out of scope;
(b) getting anything into glue-astronomy takes too long and since this blocks Cubeviz feature, I want to avoid that overhead;
(c) I would also need to add more things to its to_object because it does not support all the shapes I want here (especially sky regions).

[pllim]

(d) If I could help it, I would say we get rid of glue-astronomy altogether but that is a fiery debate for another day.

[kecnry]

ok, then let's leave it here for now (I can now definitely appreciate point b). As for points a and c, I think something in glue-astronomy is better than nothing and we could always raise NotImplementedError for things we don't need yet, but we can migrate this over if/when there is a need.

[pllim]

Can always open a follow-up issue to port the code there. I'll add that as post-merge todo.

[kecnry]

Only interactive subsets show in spatial subset dropdowns - is this always what we want? For the subset plugin, this probably makes sense since there isn't much of any information we can extract from the masked subsets (although maybe some day we'll want to be able to at least rename them there)... but is there anything preventing one of these masked subsets being used as the aperture in aperture photometry? This is likely the way that static regions behaved before, so I'd be ok with opening a new ticket and considering this out-of-scope, if you'd rather.

Also, is this the expected numbering behavior for the following snippet? I would've expected "Subset 1", "MaskedSubset 2", "MaskedSubset 3" (in other words, are the numbering supposed to be independent or shared)?

from regions import PixCoord, CirclePixelRegion, PolygonPixelRegion

imviz.load_regions([CirclePixelRegion(center=PixCoord(x=6, y=2), radius=5),
                                 PolygonPixelRegion(vertices=PixCoord(x=[1, 2, 2], y=[1, 1, 2])),
                                 PolygonPixelRegion(vertices=PixCoord(x=[2, 3, 3], y=[2, 2, 3]))])

[pllim]

Just got to your review now, Kyle. Thanks for your patience! I will comment in multiple parts as I work through the comments. And I will apply your suggestions in local commit (since I have rebased locally to resolve conflicts before I saw your suggestions) but I will credit you as co-author. Hope that is okay.

This looks like it deprecates but doesn't actually break existing API, is that right?

Yes. As you can see in the diff in test files, changes to the deprecated functionality are minimal. There is a bug fix to prevent bad mask from creating ghost Subsets but I don't see it worth mentioning in change log since the method is deprecated anyway and new API with the fix is still new API.

Do we need to file a follow-up ticket/reminder to actually remove the deprecated methods?

Yes. Over at astropy, we open a follow-up issue to actually remove the deprecated functionality after at least 2 major releases, so I intend to follow similar practice here.

[pllim]

Only interactive subsets show in spatial subset dropdowns - is this always what we want?

Not sure what you mean. Your screenshot shows non-interactive MaskedSubset too. If you are talking about the Subset Tool plugin, I think there is a known bug that the non-interactive subsets do not show until after you interactively draw another one after you have loaded them. I thought there was a ticket but I cannot find it now. Maybe @rosteen remembers.

is there anything preventing one of these masked subsets being used as the aperture in aperture photometry?

Yes. photutils only takes certain aperture shapes. Space Invader isn't one of them, but maybe we can convince @larrybradley otherwise?

I would've expected "Subset 1", "MaskedSubset 2", "MaskedSubset 3" (in other words, are the numbering supposed to be independent or shared)?

I purposely made them independent. I don't see why they need to share numbering but I could be convinced otherwise.

[kecnry]

Ok, I think all my questions were answered or addressed, thanks! Anything remaining can be iterated in the future based on user-feedback.

And just to clarify one thread:

Only interactive subsets show in spatial subset dropdowns - is this always what we want?

Not sure what you mean. Your screenshot shows non-interactive MaskedSubset too. If you are talking about the Subset Tool plugin, I think there is a known bug that the non-interactive subsets do not show until after you interactively draw another one after you have loaded them.

I meant for all plugin subset dropdowns (subset tool, model fitting, aperture photometry, etc). But your answer regarding aperture photometry suggests maybe this is desired behavior, at least for now.

[ojustino]

I tested this branch using the example FITS image and DS9 regions file from the ticket. Although all the regions are off the image, I didn't notice anything working differently than intended since Glue will still load those. I edited the regions file so a a few of their coordinates would land on frame and tried again, also with no issues. Both notebooks still work.

The tests added make sense, but should there be others for load_regions_from_file() that cover scenarios where region_format != 'ds9'? I guess those would really be a test that Regions.read() works as we expect.

[pllim]

cover scenarios where region_format != 'ds9'?

That is already tested in regions and we have not been asked to support anything other than DS9 files thus far, so I would say no.

[ojustino]

Thank you for the changes, @pllim. My last suggestions are just about wording, so I'll approve and leave their resolution to you.

[pllim]

I opened follow-up issues. They are linked in the original post above. FYI. Thanks again for the thorough reviews!