ensure footprints display when added before linking #2797

gibsongreen · 2024-04-11T15:45:45Z

Description

This PR is to address when footprints are added via API prior to linking by WCS. Only the last footprint added was visible in the viewer once linked, even when all footprints are set to be visible.

Screen.Recording.2024-04-11.at.11.42.21.AM.mov

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 · 2024-04-11T16:14:45Z

Codecov Report

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

Project coverage is 88.86%. Comparing base (a6de416) to head (ee1d01c).
Report is 4 commits behind head on main.

❗ Current head ee1d01c differs from pull request most recent head 40ed3a4. Consider uploading reports for the commit 40ed3a4 to get more accurate results

Files	Patch %	Lines
...viz/configs/imviz/plugins/footprints/footprints.py	90.47%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2797      +/-   ##
==========================================
+ Coverage   88.82%   88.86%   +0.03%     
==========================================
  Files         111      111              
  Lines       16879    16894      +15     
==========================================
+ Hits        14993    15013      +20     
+ Misses       1886     1881       -5

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

kecnry · 2024-04-11T17:02:15Z

jdaviz/configs/imviz/plugins/footprints/footprints.py

+        # When fp(s) added via API before WCS link. Overlays visibility & is_active
+        # can be True, but only last fp will display. This ensures all fp(s) display
+        if not self.is_pixel_linked:
+            for choice in self.overlay.choices:
+                self.overlay.selected = choice


can we call the method internally so that the selected choice isn't affected (I'm assuming this works because it triggers the observe on overlay_selected)? If that isn't possible, can we at least restore the user-selection after this for-loop?

It is triggering the observe. I am seeing that without changing the selected choice and calling the method internally, the only overlay that displays is the last one added based on preset_args_changed.

can we have the internal method take the name of the overlay and default to overlay_selected so that we can trigger the logic without changing the selection?

see gibsongreen#1 for a rough idea of how this could work (although I didn't have a test case, so haven't tested if it still fixes the bug - a regression test would be great if possible!)

gibsongreen · 2024-04-11T19:40:36Z

jdaviz/configs/imviz/plugins/footprints/footprints.py

+        restore_choice = self.overlay.selected
+        if not self.is_pixel_linked:
+            for choice in self.overlay.choices:
+                self.overlay.selected = choice
+        self.overlay.selected = restore_choice


I did see that if you go from pixel -> wcs (change the overlay selected to any but the last) -> pixel the overlay selected would go to the last added as well, so this should now take care of that.

kecnry · 2024-04-16T16:49:13Z

jdaviz/configs/imviz/plugins/footprints/footprints.py

+            for choice in self.overlay.choices:
+                if len(self.overlay.choices) > 1:
+                    self._change_overlay(overlay_selected=choice)


if there aren't any choices, then the for-loop shouldn't even enter. It might also be a good idea to add a comment here so we don't think this is changing the selection (feel free to edit the text to whatever you think makes sense).

Suggested change

for choice in self.overlay.choices:

if len(self.overlay.choices) > 1:

self._change_overlay(overlay_selected=choice)

for choice in self.overlay.choices:

# trigger the update without actually changing the user-selection

self._change_overlay(overlay_selected=choice)

When Imviz is instantiated in a notebook or test, the default overlay is temporarily in self.overlay.choice(and it is NoneType at this time). So to make sure that a TypeError exception isn't thrown I added this conditional.

I will add a comment now for some clarity on what this loop is doing.

so it enters the loop and then the choice disappears before _change_overlay is called?

The default does get passed into _change_overlay, it does go through creating a new entry, at the end of creating a new entry, since there is now 1 entry, the overlay tries to center_on_viewer. And when this happens the viewer.get_center_skycoord is what is actually NoneType (I mistakenly said it was the overlay).

In the case that no overlay is added in the API before WCS linking, the default that gets created does still center on the viewer (so this does still trigger correctly if there is only 1 overlay).

kecnry

LGTM pending the one comment I left (which isn't crucial). Thanks for tracking this down!

jdaviz/configs/imviz/plugins/footprints/footprints.py

rosteen

Approved but I'd prefer to see tests pass after #2809 gets merged before merging this.

Edit: eh, I guess as long as it's only the one know failure, go ahead and merge.

gibsongreen · 2024-04-17T14:30:50Z

Approved but I'd prefer to see tests pass after #2809 gets merged before merging this.

Edit: eh, I guess as long as it's only the one know failure, go ahead and merge.

I just made a small change so this will go through the CI now and make sure they all pass!

… is updated

… before linking

#2815) Co-authored-by: Gilbert Green <42986583+gibsongreen@users.noreply.github.com>

) * when footprint added before linking, make sure they display when link is updated * add change log * add ability to restore overlay selection * call _preset_args_changed instead of changing selection * internalize bug fix and add test coverage * add comment explaining link change loop * change fp to footprint in comments * address case where default overlay is created in init * change np.ones() to np.ones(()) for testing * trigger CI --------- Co-authored-by: Kyle Conroy <kyleconroy@gmail.com>

gibsongreen requested review from rosteen, javerbukh, pllim, kecnry, bmorris3, haticekaratay and cshanahan1 as code owners April 11, 2024 15:45

gibsongreen added this to the 3.9.1 milestone Apr 11, 2024

github-actions bot added imviz plugin Label for plugins common to multiple configurations labels Apr 11, 2024

gibsongreen added bug Something isn't working plugin Label for plugins common to multiple configurations 💤 backport-v3.9.x on-merge: backport to v3.9.x and removed plugin Label for plugins common to multiple configurations labels Apr 11, 2024

kecnry reviewed Apr 11, 2024

View reviewed changes

gibsongreen commented Apr 11, 2024

View reviewed changes

gibsongreen force-pushed the fp_api_before_wcs branch from ca3c497 to 00f0a17 Compare April 16, 2024 16:15

kecnry reviewed Apr 16, 2024

View reviewed changes

kecnry approved these changes Apr 16, 2024

View reviewed changes

kecnry added the Ready for final review label Apr 16, 2024

rosteen reviewed Apr 16, 2024

View reviewed changes

jdaviz/configs/imviz/plugins/footprints/footprints.py Outdated Show resolved Hide resolved

rosteen approved these changes Apr 16, 2024

View reviewed changes

gibsongreen and others added 6 commits April 17, 2024 11:37

when footprint added before linking, make sure they display when link…

8402f7a

… is updated

add change log

09baaf4

add ability to restore overlay selection

036bd0d

call _preset_args_changed instead of changing selection

3959b41

internalize bug fix and add test coverage

dfa0f76

add comment explaining link change loop

8893c45

gibsongreen added 4 commits April 17, 2024 11:37

change fp to footprint in comments

43a684e

address case where default overlay is created in init

0bf2e23

change np.ones() to np.ones(()) for testing

1f240db

trigger CI

40ed3a4

gibsongreen force-pushed the fp_api_before_wcs branch from ee1d01c to 40ed3a4 Compare April 17, 2024 15:44

gibsongreen merged commit 7e3fb1a into spacetelescope:main Apr 17, 2024
13 of 14 checks passed

meeseeksmachine pushed a commit to meeseeksmachine/jdaviz that referenced this pull request Apr 17, 2024

Backport PR spacetelescope#2797: ensure footprints display when added…

29fb270

… before linking

meeseeksmachine mentioned this pull request Apr 17, 2024

Backport PR #2797 on branch v3.9.x (ensure footprints display when added before linking) #2815

Merged

kecnry pushed a commit that referenced this pull request Apr 17, 2024

Backport PR #2797: ensure footprints display when added before linking (

79ba339

#2815) Co-authored-by: Gilbert Green <42986583+gibsongreen@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ensure footprints display when added before linking #2797

ensure footprints display when added before linking #2797

gibsongreen commented Apr 11, 2024 •

edited

Loading

codecov bot commented Apr 11, 2024 •

edited

Loading

kecnry Apr 11, 2024

gibsongreen Apr 11, 2024

kecnry Apr 11, 2024

kecnry Apr 12, 2024

gibsongreen Apr 11, 2024

kecnry Apr 16, 2024

gibsongreen Apr 16, 2024

kecnry Apr 16, 2024

gibsongreen Apr 16, 2024 •

edited

Loading

kecnry left a comment

rosteen left a comment •

edited

Loading

gibsongreen commented Apr 17, 2024 •

edited

Loading

ensure footprints display when added before linking #2797

ensure footprints display when added before linking #2797

Conversation

gibsongreen commented Apr 11, 2024 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

codecov bot commented Apr 11, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibsongreen Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

kecnry left a comment

Choose a reason for hiding this comment

rosteen left a comment • edited Loading

Choose a reason for hiding this comment

gibsongreen commented Apr 17, 2024 • edited Loading

gibsongreen commented Apr 11, 2024 •

edited

Loading

codecov bot commented Apr 11, 2024 •

edited

Loading

gibsongreen Apr 16, 2024 •

edited

Loading

rosteen left a comment •

edited

Loading

gibsongreen commented Apr 17, 2024 •

edited

Loading