Conversation
ac8a8d4 to
e7e8cf1
Compare
kecnry
left a comment
There was a problem hiding this comment.
Exciting to see this all come together!
- I wonder if it makes sense since this is in a custom/temporary toolbar to NOT nest these in a single dropdown?
- Are there plans for skewer with multiselect (all intersecting footprints get added/removed from the selection)? How would that fit into this?
|
Ok, we'll just want to try to get that out before the next release to avoid breaking behavior changes. But so long as that fits into your schedule, I'm fine getting this in as it is now. Nice work! |
f4d814c to
38b8dc0
Compare
bmorris3
left a comment
There was a problem hiding this comment.
This looks great @haticekaratay. Let's hold off on merging this PR, and implement multiselection before merge.
There was a problem hiding this comment.
This is ✨awesome.
Question for @kecnry et al.: every time you select/deselect a footprint preview, the astroquery loader sends another query for products. These queries aren't particularly fast, and it's quite frustrating to wait for the query results in between each click.
Can we stop auto-querying on changes in selection, and start using a Query Products button instead, or similar?
|
Can we just debounce/throttle the calls in a separate thread so they don't block additional clicks instead? |
I think that'll look like intermittent and unexpected periods of frozen app for the user. MM and especially the table widget are too slow to do this in the same kernel thread. |
|
if its not blocking then it should only show as the progressbar in the loader (we can also gray out the table and everything below it during an update, if necessary). I just want to avoid adding more steps to click, if possible, especially since those would then need to be done when using the API as well. |
|
It is blocking: Screen.Recording.2026-02-03.at.10.19.33.mov |
|
Yes, its currently blocking, but we could/should make it not blocking |
bmorris3
left a comment
There was a problem hiding this comment.
Notes for future work that does not need to be addressed in this PR:
- the object loader UI button "enable footprint selection tools" (toggled by
custom_toolbar_enabled) has no public API equivalent. Let's make one. - should "enable footprint selection tools" be default True? (we can wait until we've got a more feature complete workflow to do this)
- in the screengrab I posted above, it seems like the products aren't shown until a second footprint selection is made. seems like a bug?
- the ipyvuetify table widget is very slow to load long tables. Are there faster alternatives that don't require multithreading?
- I'd advocate for manual triggers for data product queries via API/UI. it's inefficient to make a new query on every footprint click.
- the ipyvuetify table row update is blocking but you wouldn't know unless you were looking at the footer. this needs a progress bar if we keep automatic data product list queries.
- in the current implementation, we allow the user to load the observation table from API, but we automatically download product lists on selection events. We should expose constraints on the data product queries, like "_cal files" or "L2", etc. That limits the query results and the response time.
- the current footprint preview click behavior always adds the footprints to the existing selection. this makes deselection tricky. I'd advocate for having, e.g., shift+click mean "add to selected footprints" and the default click mean "select only the footprints that meet the matching criteria for this tool, and deselect other selections"
- the implementation in this PR applies to the object loader. the footprint preview/select features should also be added to the astroquery loader.
|
Great, thanks @bmorris3! Please make sure that anything that won't be done here gets a follow-up ticket for whatever team is responsible for taking it on.
agreed - we will want to do this in a general way so I think we postponed it when first building the infrastructure to see where else we'd use it. But between this and #3994, I think its time.
I would probably argue against this unless we can find some clean trigger to do it and safety mechanism to not overwrite another open toolbar from another plugin, etc. We definitely don't want it to be
this might be worth checking if it exists before this PR or not, and if it was somehow introduced just fix it here
I believe there is already a ticket for this (@camipacifici suggested the same)
we now no longer have a concern about multiple tools (but let's not go crazy). I agree we need a way that is flexible and not confusing and there are lots of different combinations of how a user might want to use this. #3994 also will introduce the ability to add generic widgets in the custom toolbar, so you could even have simple "settings" for how a tool should act (a toggle for whether to use multi-select, etc).
the code looks like its in the base class already, so I'm surprised that wouldn't work, but we definitely should make sure it does. |
bmorris3
left a comment
There was a problem hiding this comment.
Thanks @haticekaratay!
The only potentially actionable comment in the review is the first one. We're good to merge when that's answered.
| # Toggle all found footprints as a group | ||
| # If ALL are selected, deselect ALL; otherwise select ALL | ||
| selected_indices_set = set(selected_indices) | ||
| if selected_indices_set.issubset(currently_selected): | ||
| # All found footprints are already selected - deselect them all | ||
| currently_selected -= selected_indices_set | ||
| else: | ||
| currently_selected.add(selected_idx) | ||
| # At least one is not selected - select them all | ||
| currently_selected |= selected_indices_set |
There was a problem hiding this comment.
This is the block we'll need to update to support 'click to deselect' in a near-future PR.
There was a problem hiding this comment.
Just want to say that these tests are so clear, thank you.
| "s3fs>=2024.10.0", | ||
| "joblib>=1.3.0", | ||
| "ipyvuedraggable>=1.1.0", | ||
| "spherical-geometry", |
There was a problem hiding this comment.
Note: this dependency is currently needed to do proper contains and intersects operations for points and polygons on a sphere. Sedona has an implementation of spherical region operations in a PR to astropy regions (astropy/regions#618), which could make this dependency unnecessary at some point in the future, though we will have to test if the Python implementation in regions is too slow for our use compared to the C implementation in spherical-geometry.
There was a problem hiding this comment.
For most supported workflows within jdaviz, the viewer FOV is expected to be <<1 deg. At that scale, one could argue that the small angle approximation could be assumed valid and we could use cartesian polygon overlap instead of spherical. We haven't made that assumption because it breaks down at the poles of any coordinate frame.
I can see the products with a single click, but it is slow. Screen.Recording.2026-02-03.at.11.28.50.AM.mov |
22e7f8f to
68dc390
Compare
68dc390 to
aa03ad7
Compare
Description
This PR adds a new
skewerselection mode for interactive footprint selection in the loaders infrastructure. The skewer mode selects footprints by evaluating whether a clicked sky coordinate lies within a footprint’s spherical polygon, and is available in addition to the existing nearest-edge footprint selection behavior.This work was initially developed for the Footprints plugin (see #3794), but has since been refactored and integrated into the loaders infrastructure. By implementing this in loaders, the skewer selection mode is now available wherever loader-based footprint selection is used.
Screen.Recording.2026-01-20.at.1.55.09.PM.mov
To make it easier to visualize how the skewer selection mode works, check the demo with well-separated footprints.
When clicking inside overlapping footprints, the skewer mode automatically selects the one with the smallest polygon area. This ensures that when a smaller footprint is nested within a larger one, clicking in the overlap region will correctly select the more specific (smaller) footprint.
Screen.Recording.2026-01-20.at.3.06.50.PM.mov
Change log entry
CHANGES.rst? If you want to avoid merge conflicts,list the proposed change log here for review and add to
CHANGES.rstbefore merge. If no, maintainershould add a
no-changelog-entry-neededlabel.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.
triviallabel.cache-download.ymlworkflow?