Footprints plugin

[kecnry]

Description

IMPORTANT: this PR currently require pysiaf to be installed or the plugin is disabled. We may want to either remove this dependency entirely or make it an official dependency that is installed with jdaviz.

This pull request implements a new plugin in Imviz to plot an arbitrary number of user-configured footprints from any JWST instrument, adapted from code written by @melanieclarke in https://github.com/spacetelescope/jwst_novt. In the future, this could easily be extended to other instruments or to a generic "overlays" or "regions" plugin.

Basic support for adding and customizing any number of footprints (NOTE some names have been changed to be more general with "overlay"/"preset" instead of "footprint"/"instrument"):

Screen.Recording.2023-08-04.at.2.16.46.PM.mov

Logic for handling multiple viewers:

Screen.Recording.2023-08-04.at.2.20.15.PM.mov

Handling lack of WCS and ability to recenter footprint:

Screen.Recording.2023-08-11.at.10.09.47.AM.mov

Footprints plugin docs

This PR also brings the EditableSelectPluginComponent (which allows the user to name/rename/add/delete an arbitrary number of footprint overlays) originally implemented in spacetelescope/lcviz#19 upstream into jdaviz, as well as implementing the concept of inapplicable_attrs to use within the plugin user APIs (if we adopt this here, we could follow-up with improvements to plot options and other plugins where some traitlets are not always applicable). spacetelescope/lcviz#36 will then update lcviz to depend on the implementation here so that we do not have two duplicate implementations.

TODO

• docs
• test coverage
• screen recordings of inapplicable_attrs (or strip into separate PR since no longer needed now that all instruments are generalized and support the same input arguments)
• decide what to do with pysiaf dependency
• consider renaming to "overlays" or "regions" for future expansion
• miri & niriss support
• finalize list of apertures (and possibly make user-selectable?), including aperture used for centering
• decide whether to have NIRCam:short vs long or a separate "path" dropdown that is only applicable for NIRCam (could use Plugin user API: inapplicable attrs #2347)
• handle change in reference data (and plan what will need to be done with ENH: Image rotation via WCS-only layers #2179)
• fix renaming resets overlay color
• rebase and confirm no issues with plot options histogram being laggy (see Footprints plugin #2341 (comment))

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

[melanieclarke]

This looks great! I really like being able to add and configure as many footprints as I want, from all the instruments.

A couple of picky details:

• NIRCam and NIRSpec are labeled as 'NIRcam' and 'NIRspec' - the capitalization should be fixed
• in the jwst_footprints code, there are variables named 'nrc_', which looks like a holdover from my code that treated NIRSpec and NIRCam separately. The 'nrc' was short for NIRCam. The variable names might be less confusing without this prefix.

And a couple comments:

• The footprints plugin seems to interact badly with the plot options histogram, particularly when the "Limit histogram to current zoom limits" option is on. Updates for the histogram are very slow on zooming and the footprints flash on and off repeatedly while waiting.
• I can't seem to get the footprints to show up in my particular use case in the NOVT application. I'm not sure if it's my custom configuration or layout or something else. I'll keep checking.

[kecnry]

Thanks for the feedback @melanieclarke

NIRCam and NIRSpec are labeled as 'NIRcam' and 'NIRspec' - the capitalization should be fixed

Fixed

in the jwst_footprints code, there are variables named 'nrc_', which looks like a holdover from my code that treated NIRSpec and NIRCam separately. The 'nrc' was short for NIRCam. The variable names might be less confusing without this prefix.

Ah yes, I meant to clean that up after generalizing to all instruments and then forgot. Should be fixed now too, thanks!

The footprints plugin seems to interact badly with the plot options histogram, particularly when the "Limit histogram to current zoom limits" option is on. Updates for the histogram are very slow on zooming and the footprints flash on and off repeatedly while waiting.

I think this will be fixed by rebasing on top of #2326 - I'll rebase soon after I get test coverage up and confirm that is the case.

I can't seem to get the footprints to show up in my particular use case in the NOVT application. I'm not sure if it's my custom configuration or layout or something else. I'll keep checking.

Ok, let me know if there's anything I can do to help!

[melanieclarke]

@kecnry - Perfect, thanks!

For the NOVT display, it looks like it's a configuration issue. If I use the Imviz configuration without any customization, the footprints work fine. My footprints appear right on top of yours, with the same parameters, as they should! I'll sort out the config issue on my side.

[codecov]

Codecov Report

Patch coverage: 91.52% and project coverage change: +0.04% 🎉

Comparison is base (45b0127) 90.66% compared to head (a2eed6e) 90.71%.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2341      +/-   ##
==========================================
+ Coverage   90.66%   90.71%   +0.04%     
==========================================
  Files         153      157       +4     
  Lines       17576    17969     +393     
==========================================
+ Hits        15935    16300     +365     
- Misses       1641     1669      +28     

Files Changed	Coverage Δ
jdaviz/app.py	85.04% <ø> (ø)
jdaviz/core/template_mixin.py	90.13% <73.62%> (-0.88%)	⬇️
...configs/imviz/plugins/footprints/preset_regions.py	88.57% <88.57%> (ø)
...viz/configs/imviz/plugins/footprints/footprints.py	96.82% <96.82%> (ø)
jdaviz/configs/imviz/plugins/__init__.py	100.00% <100.00%> (ø)
...daviz/configs/imviz/plugins/footprints/__init__.py	100.00% <100.00%> (ø)
.../imviz/plugins/footprints/tests/test_footprints.py	100.00% <100.00%> (ø)
jdaviz/configs/imviz/plugins/viewers.py	90.50% <100.00%> (+0.71%)	⬆️
jdaviz/core/marks.py	90.32% <100.00%> (+0.32%)	⬆️
jdaviz/core/user_api.py	92.85% <100.00%> (+0.12%)	⬆️

... and 1 file with indirect coverage changes

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

[kecnry]

Few open questions to still consider during review:

• consider renaming plugin to "overlays" or "regions" for future expansion ("From File..." or API import of regions support, etc) (will keep the plugin name but generalize the API)
• decide what to do with pysiaf dependency (currently does not get installed with jdaviz, but plugin will be disabled until it is manually installed)
• finalize list of apertures (and possibly make user-selectable?), including aperture used for centering
• decide whether to have NIRCam:short vs long or a separate "path" dropdown that is only applicable for NIRCam (could use Plugin user API: inapplicable attrs #2347) (will keep as-is)
• follow-up tickets for any work needed when ENH: Image rotation via WCS-only layers #2179 is rebased on top of this (footprint WCS-mapping will need to be updated on change to reference data)

[pllim]

I thought we agreed to bundle footprints data files and not rely on pysiaf at all? All we need is the vector points for polygons, no?

[kecnry]

I thought we agreed to bundle footprints data files and not rely on pysiaf at all? All we need is the vector points for polygons, no?

We would also need a replacement for pysiaf.utils.rotations.attitude if we are to move the data locally and try to remove pysiaf as a dependency entirely.

[pllim]

Just these 12 lines? I'd rather we bundle this and throw in the pysiaf license here as acknowledgment than requiring the whole package.

    v2d = v2 / 3600.0
    v3d = v3 / 3600.0

    # Get separate rotation matrices
    mv2 = rotate(3, -v2d)
    mv3 = rotate(2, v3d)
    mra = rotate(3, ra)
    mdec = rotate(2, -dec)
    mpa = rotate(1, -pa)

    # Combine as mra*mdec*mpa*mv3*mv2
    m = np.dot(mv3, mv2)
    m = np.dot(mpa, m)
    m = np.dot(mdec, m)
    m = np.dot(mra, m)

    return m

p.s. Do they mean "altitude" instead of "attitude"?

[rosteen]

The footprints shouldn't care that linking is set to Pixel if there's only one dataset loaded in the viewer, right? Right now the footprints plugin remains disabled even if I unload all but one image from the viewer:

[kecnry]

The footprints shouldn't care that linking is set to Pixel if there's only one dataset loaded in the viewer, right?

That's a good point - right now I do this logic based on how many are in the app's data collection. Alternatively I guess we could have the viewer dropdown to filter based on the number loaded and the linking type, but maybe it makes more sense to push that down the road to the work to better support the pixel-linked case (which in turn is waiting on the rotation work to get merged which should simplify this greatly and probably would mean removing a lot of these guardrails anyways). If you're ok with that, I'll add a comment to that ticket 🐱 so that we can consider this case as well.

[rosteen]

If we're potentially removing guardrails later then I'd be fine with this case being future work, as long as you add the comment to the appropriate ticket. In that case, consider my already green review to be extra green.

[kecnry]

Yes, I think we definitely want to support the pixel-linking case better in the future, its just really not clear how to do so now in a way that will still be compatible with the ongoing rotation work, so I think for now very conservative guardrails make sense so that we can move forward with other planned work for this plugin, and I've added your and @pllim's comments so we make sure we support all (possible) use-cases for pixel-linking in a hopefully self-consistent and intuitive way down the road.

Thanks everyone for the very useful thoughts and reviews!