Footprints plugin: handle subsets/markers when linking by WCS

[kecnry]

Description

Previously, pressing the button in the footprints plugin to align by WCS resulted in a traceback if subsets or markers existed. This now redirects to the orientation plugin itself in cases where the link change fails for either of these reasons. In the future, we may want to use the same logic as in the orientation plugin to detect these in advance and either add additional buttons or language to allow deleting the subsets/markers directly from footprints.

This PR also updates some language in the alerts in the orientation plugin to match recent API changes.

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. Bugfix milestone also needs an accompanying backport label.
• After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

[codecov]

Codecov Report

Attention: Patch coverage is 60.00000% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.75%. Comparing base (1f57249) to head (be76347).

Files with missing lines	Patch %	Lines
...viz/configs/imviz/plugins/footprints/footprints.py	60.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3276      +/-   ##
==========================================
- Coverage   88.76%   88.75%   -0.01%     
==========================================
  Files         125      125              
  Lines       18956    18960       +4     
==========================================
+ Hits        16826    16828       +2     
- Misses       2130     2132       +2     

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

[rosteen]

There seems to be a problem with this workflow. As you can see in the recording, after I click "Link by WCS" in footprints and then go up to Orientation to clear the subsets, the toggle has already been changed to WCS linked, but that hasn't really happened. I have to toggle to pixel and then back to WCS to get it to change.

Screen.Recording.2024-11-11.at.11.44.21.AM.mov

[kecnry]

Good catch - the latest commit detects the cases with an if-statement instead of the try-except that resulted in the traitlet changing before the error was raised/caught by the except.

[rosteen]

Working as expected now. A test would be nice, but I'm approving.

[kecnry]

This is a bit hard to test (besides just that it doesn't crash) - I'm working to see if the full mixin implementation will be easy and maybe we can just swap this out with that immediately if there aren't any roadblocks.

[kecnry]

update: there isn't any technical hurdle, but will take a bit more effort as quite a bit of code needs to move around, so I'll make a ticket for that for future follow-up (POC in #3277). Hopefully this is still an improvement to patch the bug until we can get that prioritized.

[bmorris3]

Why not?

[kecnry]

this was there before, I think from concerns by @pllim

[kecnry]

(the diff here is just from moving its position so that the "warnings" both come before the alerts with actions)

[pllim]

I forgot the exact reason now but hopefully that concern was captured somewhere in this GitHub repo. Maybe related to this?

jdaviz/jdaviz/configs/imviz/tests/test_orientation.py

Lines 74 to 81 in 5130caf

	# Both marks stay the same in sky, so this means X and Y w.r.t. reference
	# same on both entries.
	# FIXME: 0.25 offset introduced by fake WCS layer (remove AssertionError).
	# https://jira.stsci.edu/browse/JDAT-4256
	with pytest.raises(AssertionError):
	assert_allclose(mp._obj.marks["imviz-0"].x, 0)
	with pytest.raises(AssertionError):
	assert_allclose(mp._obj.marks["imviz-0"].y, 0)

[kecnry]

ah, I think you're right. That is still open, so let's leave the text in place and I'll make sure there is a link back here so that we remove the warning if/when addressing that. Thanks!