Footprints: custom regions import

[kecnry]

Description

This pull request implements the ability to load a regions file into the footprints plugin, adopting the pattern we created for searching custom catalog files in the catalogs plugin (but generalizing that logic and moving it into a reusable plugin component)

Screen.Recording.2023-08-16.at.1.23.07.PM.mov

This is also supported from the plugin user API:

fp = imviz.plugins['Footprints']
fp.open_in_tray()
fp.import_region(filename_or_regions_object)

TODO:

• support for loading regions from API (not a file)
• fix internal caching when more than one file/object loaded
• improve parser to require sky region
• test coverage (including fixing broken catalogs test)
• docs
• support for repositioning? (defer to follow-up: not really clear how this should work, we could either have offset parameters or compute the center by averaging all vertices and then a change in ra/dec would need to then be applied as delta changes to the vertices)
• make use of inapplicable attrs to hide JWST-specific traitlets when a file is selected? (possibly defer to follow-up)

Open questions:

• do we want to rename "preset" (in the API and UI) to "region"?

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: 90.81% and project coverage change: +0.06% 🎉

Comparison is base (2e61f48) 90.52% compared to head (4e82391) 90.59%.

❗ Current head 4e82391 differs from pull request most recent head 4eb05d7. Consider uploading reports for the commit 4eb05d7 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2377      +/-   ##
==========================================
+ Coverage   90.52%   90.59%   +0.06%     
==========================================
  Files         159      159              
  Lines       18236    18315      +79     
==========================================
+ Hits        16509    16593      +84     
+ Misses       1727     1722       -5     

Files Changed	Coverage Δ
jdaviz/app.py	85.25% <ø> (ø)
...configs/imviz/plugins/footprints/preset_regions.py	90.90% <ø> (+2.33%)	⬆️
...viz/configs/imviz/plugins/footprints/footprints.py	92.53% <83.60%> (-3.81%)	⬇️
jdaviz/core/template_mixin.py	90.35% <90.14%> (-0.07%)	⬇️
jdaviz/configs/imviz/plugins/catalogs/catalogs.py	86.60% <100.00%> (+0.14%)	⬆️
jdaviz/configs/imviz/tests/test_catalogs.py	73.86% <100.00%> (ø)
jdaviz/configs/imviz/tests/test_footprints.py	100.00% <100.00%> (ø)
jdaviz/core/marks.py	90.32% <100.00%> (+0.24%)	⬆️

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[pllim]

Just general design thoughts. For a generic region load to be useful:

1. It has to work in pixel linking or when only one image is loaded.
2. There should be API to grab the "footprint" object back out as proper regions shapes (not just polygon, a circle region loaded should come back out as circle shape).
3. If this functionality needs to be within this same plugin, then pysiaf needs to be optional even in this plugin. I don't want to have to install pysiaf just to load in a DS9 region file.
4. The regions API in this plugin needs to somehow tie back nicely to helper load_regions and get_regions. This needs some thought because right now those APIs are tied to subsets, but eventually if we want to get rid of subsets, we need this for less painful transition.

[kecnry]

It has to work in pixel linking or when only one image is loaded.

This is still planned, but deferred. 🐱

There should be API to grab the "footprint" object back out as proper regions shapes (not just polygon, a circle region loaded should come back out as circle shape).

This already exists as fp.overlay_regions - you should get the original imported region (no translating is currently done in the app).

If this functionality needs to be within this same plugin, then pysiaf needs to be optional even in this plugin. I don't want to have to install pysiaf just to load in a DS9 region file.

I agree - I'll copy your comment over to the ticket where we reconsider the pysiaf dependency 🐱. If anyone insists, I could make that happen now (have the plugin enabled, but just remove the jwst instruments from the dropdown)... but I would need to think about how to make that work with no other options available and if we're likely removing that logic soon anyways, I'm not sure its worth the effort and code bloat now.

The regions API in this plugin needs to somehow tie back nicely to helper load_regions and get_regions. This needs some thought because right now those APIs are tied to subsets, but eventually if we want to get rid of subsets, we need this for less painful transition.

Down the road we might want the helper methods to have an option to load directly in this plugin or as subsets (and we might also want the ability to convert regions/footprints in this plugin into subsets and vice versa), but I also think we could decide to leave them be separate. This plugin is still called "footprints", so even though it supports generic regions, the scope/context of the plugin is technically just to "overlay instrument footprints".

[pllim]

we might also want the ability to convert regions/footprints in this plugin into subsets and vice versa

With the linking and projection, we would have to be careful. Basically, we need to recreate some stuff that glue-core provides natively with Subsets.

[rosteen]

Overall this is looking pretty good to me, but I see an annoying formatting thing when I load a region from file. Oddly, the formatting fixes itself if I switch my mouse over to my other computer, so it might be other software on my computer somehow messing things up:

If no one else can replicate this, I'm probably ok approving.

[kecnry]

That happens when the hint breaks onto two lines... I didn't think that could happen with our minimum tray width, but let me see if I can make it more responsive.

EDIT: I thought the tray had a minimum width, but apparently that is a percentage. I think we definitely want a minimum absolute width so that we can design the UI accordingly, so I set that here at 250px, which just so happens to ensure that the hint in this case doesn't overflow to two lines 😉

[rosteen]

Looks better to me now. As I said offline, I'm concerned that the From file option is a little more hidden than I'd like, but we can iterate with Jenn in the future if we want on that sort of thing. For now I think it's good enough to have it there.

[pllim]

See kecnry#4

[kecnry]

Overall the plugin isn't very robust from many different combos of user interactions and I cannot get import_region API to work

Can you explain what situations do not work? The scope here was to allow file and API import of sky regions (pixel-linking and pixel-regions support is intentionally deferred, as previously discussed, and drawing single points may or may not be added in the future). Although there are more general use-cases, the plugin is called "footprints", so the idea is to be able to support importing custom instrument footprints, and doesn't need to support generic pixel apertures, for example.

The file dialog (in the original PR) and the API should prevent loading pixel regions, and hopefully only allow files/regions that are compatible. If that is not the case, please send any file/region that you would expect to load or expect to be prevented from loading under the scope of the PR and I will fix that.

[pllim]

Two things still confused me.

Use case 1

I used the API to load a file but plugin is not opened on GUI. Calling footprints_plugin.import_region("four_circles_test.reg") did not crash, so I thought it must have loaded something. I don't see the circles displayed, which is fine because plugin is not opened.

But I expected the circles to show if I then open the plugin. But they don't. If I call the API again while plugin is opened, then they show.

Use case 2

I load a region file. The circles show. All is well. Then I go to "Overlay" section and click "+", enter a new name, and then the checkmark. I go back to "From File..." and select a different region file to load it.

It works in notebook, but hangs in standalone/voila; i.e., the browser display (not just Imviz) grays out and unresponsive. No traceback on terminal. This is Windows 10 + Chrome. Works now in standalone too when I run it again. 🤷

[pllim]

Update: I cannot reproduce "Use case 2" so maybe it was some frontend fluke. We can ignore it for now. Would still like to know if Use Case 1 is expected behavior or not.

[kecnry]

Let me rebase on top of #2386 and make sure that that handles the mark visibility as expected

[pllim]

Thanks!

[pllim]

This is okay for now but I am not sure this is a good idea if we end up having multiple optional dependencies to support footprints for different telescopes.

[kecnry]

Agreed - this is just intended to be temporary until we make a decision on what to do with pysiaf (and other similar dependencies)

[pllim]

CI failure is unrelated and is being handled in #2399