Catalog Search: Hide markers only when clear

[pllim]

Description

This pull request is to improve performance of the Clear button in Catalog Search plugin by merely hiding the markers instead of permanently removing them from glue data collection. This idea was originally given by @kecnry .

Dev notes

To create offline catalog table on-the-fly for testing:

1. Use the single-pixel region to select a few points of interest (alt + click).
2. Use the code below to extract the sky coordinates and write them to a file.

from astropy.table import QTable
from astropy.coordinates import SkyCoord

regions = imviz.get_interactive_regions()

w = imviz.app.data_collection[0].coords  # WCS of reference data
ra = []
dec = []
for reg in regions.values():
    s = w.pixel_to_world(reg.center.x, reg.center.y)
    ra.append(s.ra.deg)
    dec.append(s.dec.deg)

sky = SkyCoord(ra, dec, unit='deg')
tbl = QTable({'sky_centroid': sky})
tbl.write('mycoord.ecsv')

For the new Imviz example notebook, I got the following file content:

# %ECSV 1.0
# ---
# datatype:
# - {name: sky_centroid.ra, unit: deg, datatype: float64}
# - {name: sky_centroid.dec, unit: deg, datatype: float64}
# meta: !!omap
# - __serialized_columns__:
#     sky_centroid:
#       __class__: astropy.coordinates.sky_coordinate.SkyCoord
#       dec: !astropy.table.SerializedColumn
#         __class__: astropy.coordinates.angles.Latitude
#         unit: &id001 !astropy.units.Unit {unit: deg}
#         value: !astropy.table.SerializedColumn {name: sky_centroid.dec}
#       frame: icrs
#       ra: !astropy.table.SerializedColumn
#         __class__: astropy.coordinates.angles.Longitude
#         unit: *id001
#         value: !astropy.table.SerializedColumn {name: sky_centroid.ra}
#         wrap_angle: !astropy.coordinates.Angle
#           unit: *id001
#           value: 360.0
#       representation_type: spherical
# schema: astropy-2.0
sky_centroid.ra sky_centroid.dec
9.41977607954574 -33.70046398027275
9.406650484459085 -33.70554633733875
9.420681964416504 -33.70592621977641
9.424663929881714 -33.705848835485774
9.433416595882894 -33.7192933684372
9.436731481098331 -33.715615528617896
9.440798418206091 -33.712066715141184

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?
• After merge, any internal documentations need updating (e.g., JIRA, Innerspace)? 🐱

[codecov]

Codecov Report

Base: 87.82% // Head: 87.82% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (037ca25) compared to base (1acc87d).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #1774   +/-   ##
=======================================
  Coverage   87.82%   87.82%           
=======================================
  Files          95       95           
  Lines       10167    10178   +11     
=======================================
+ Hits         8929     8939   +10     
- Misses       1238     1239    +1     

Impacted Files	Coverage Δ
jdaviz/configs/imviz/helper.py	96.36% <100.00%> (+0.03%)	⬆️
jdaviz/configs/imviz/plugins/catalogs/catalogs.py	91.86% <100.00%> (+0.64%)	⬆️
jdaviz/utils.py	89.38% <0.00%> (-0.89%)	⬇️

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[kecnry]

Just a few questions before I do a full review. My original thought was that would could hide the layer before making the call to remove the markers, just so that it feels more responsive to the user... but I guess as long as this data collection entry is not exposed to the user, this might do the trick as well.

• is this actually faster than removing?
• will future searches overwrite the data and show the markers (of just the new search)?

[pllim]

is this actually faster than removing?

Theoretically, yes. But I also don't have the exact workflow used in "stress test". How should I test this manually?

will future searches overwrite the data and show the markers (of just the new search)?

I think so but I should double check.

[pllim]

@kecnry , overwriting the hidden markers (without completely removing first) seems ok. I tested it by created two ECSV files, one with 7 markers, another with just 3. Then I load one and then the other with this PR branch, seems to work as expected.

[camipacifici]

Very fast! Looks good to me.

[kecnry]

Probably out of scope, but I noticed that the data-collection entry for the "catalog results" shows in the data menu but is not recognized as loaded into the viewer. I can create a separate issue for this, but would you expect it to be hidden entirely or just shown as loaded and visible? Should it show as a layer in the legend and plot options?

Currently the following workflow results in unexpected behavior:

• search catalog that returns results (from file, for example)
• clear the markers (they are hidden, data collection entry still exists)
• do another search that returns zero results (nothing shows, but "catalog results" still contains the original search results)

Should this either set "catalog results" to be empty? Or should the original hide also delete the entry from the data collection as before (but just hide first so it feels snappier to the user)?

[kecnry]

this isn't public API yet, so we can defer this decision if you prefer, but I wonder if separate hide and clear/remove methods would be more intuitive to the user (and then we could have the UI button always call hide)?

[pllim]

If you prefer "hide first, clear in the background no matter what" I can do that, but keep in mind that other operations will be blocked until that is done, so that lag is still there but just deferred. Is that what you want?

For the undefined behaviors:

• Markers not showing in Data -- Yes, that is existing behavior. I noticed that at the time of first implementation. I do not know what the correct behavior should be. In Ginga, the markers are canvas overlay object, not part of the real contents. But here, I need to create a table to link them, so they become part of the real contents though they are really not... 🤯 But addressing that is out of scope here.
• Whether markers should be purged or just hidden -- I haven't spent a lot of time thinking about caching for this plugin yet. While it might be trivial for local file IO to keep purging and reloading, expensive remote query is quite another matter. Thinking about caching stuff properly is also out of scope here.

[kecnry]

If you prefer "hide first, clear in the background no matter what" I can do that, but keep in mind that other operations will be blocked until that is done, so that lag is still there but just deferred. Is that what you want?

We could probably do it with a message, but fair enough, probably not worth the extra effort and we can decide the expected behavior in the data-menu separately once the behavior here is converged (perhaps we just skip this entry in the data-menu entirely, in which case the user doesn't really realize the difference unless they dig into the API).

Whether markers should be purged or just hidden -- I haven't spent a lot of time thinking about caching for this plugin yet. While it might be trivial for local file IO to keep purging and reloading, expensive remote query is quite another matter. Thinking about caching stuff properly is also out of scope here.

Ok, I agree. But if a search yields zero results and then the data collection object is manually set to be visible, would you expect to see nothing or see the results of the previous non-zero-results search (which is what currently happens)? Again, perhaps this point becomes moot if we decide to skip the entry in the data menu.

[pllim]

if we decide to skip the entry in the data menu

Not sure if that is possible right now due to #773 . So if you create a new viewer, the markers won't show by default even though they are in data collection, so that menu is then useful to force them to show. 🙈

[kecnry]

In the if query_region_result is None: block, could we either update the data_collection entry to be empty or force the full clear? I think that would get rid of this weird behavior case and then we can decide what to do with the data menu as a follow-up effort.

[pllim]

Re: #1774 (comment)

@kecnry , I added a second commit to implement what I think you meant. Does it look better now?

[kecnry]

Yes, thanks, I think this will do the trick for now and any remaining questions can be addressed if/when we refactor the catalog and work towards exposing the API. The apparent speed-up is definitely appreciated and worth getting in in the meantime!

[pllim]

Thanks for the reviews!