-
Notifications
You must be signed in to change notification settings - Fork 88
Skewer selection #3794
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Skewer selection #3794
Conversation
f4c17cd to
90a9ec5
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its good to see movement again on footprints, thanks! 🎉
pyproject.toml
Outdated
| "pyvo>=1.5.3", | ||
| "s3fs>=2024.10.0" | ||
| "s3fs>=2024.10.0", | ||
| "spherical-geometry" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we're trying to be very intentional about adding additional dependencies, although this one seems to be maintained internally and pretty lightweight. Do we need any specific version?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need a specific version, at the moment. We're also cautious about adding deps, but implementing our own spherical-geometry in jdaviz would be strange.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is very similar to our current catalog_select icon. Is this just a placeholder for a custom icon or should we plan out how to differentiate between these?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! That’s a placeholder at the moment. I’m getting Jenn’s input on the right icon for this new tool.
|
|
||
|
|
||
| @viewer_tool | ||
| class SkewerSelectFootprint(CheckableTool, HubListener): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instead of a separate tool, was a modification of the current tool considered (ctrl+click, keypress event, etc) considered?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That’s a good point. For this case, we wanted it to be a separate tool. The interaction is different enough (skewer vs. nearest footprint) that we thought it would be clearer for users to have its own tool. cc: @bmorris3
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the interest of maximizing discoverability, is it possible to have the tool menu show a selection icon with an expand arrow, which opens a submenu of footprint selection options?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we already have the dropdowns in the toolbar, I'm not sure we want another level of nesting... but maybe we need somewhere for tool settings?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another take: should there be a new 'selection options' tool, with a mouse arrow icon, that expands with the selection options? I don't think these choices naturally belong in a group with blinking and contrast/bias.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could split that "category" into two, but that increases the probability of horizontal overflow issues (which are not currently handled well) for narrow viewers. We also had a long-term plan (not currently prioritized, afaik) to allow activating viewer tools from the plugins themselves which I think would really help in this case as the footprint plugin could then put the tools in more context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
|
||
| def _handle_skewer_click(self, data): | ||
| """Spherical (great-circle) selection: only selects if the click is INSIDE a footprint. | ||
| If multiple overlays contain the click, picks the one with smallest area. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe this should also be in the tooltip? I was wondering whether it would select multiple, closest, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The longer-term goal is to select all matches, once footprint multiselect is compatible with the Footprints plugin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok, depending on the resolution of the conversation related to that... if this stays with the single-select at least for now, it might still make sense to describe in the tool description/tooltip and we can always modify that once multiselect is included.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following comment applies to the implementation as it is written, where multiselection isn't an option. I'd argue that we can't have true skewer selection without multiselection.
While testing, I think I noticed a bug when the footprint has multiple marks. In this video, I'm alternately clicking on 1) the overlap between the green MIRI footprint, the magenta FGS footprint, and the orange NIRSpec footprint, and 2) the overlap between the green MIRI footprint, the magenta FGS footprint.
smallest-area.mov
For click (2), the skewer selection tool is choosing the smallest area mark, which is the green MIRI footprint (✅).
For (1), skewer select appears to choose the mark with the smallest area. The smallest mark which contains the click coordinate is determined to be NIRSpec, even though NIRSpec has the biggest FOV of all the choices here. Is it picking NIRSpec because it's comparing the mark area, which is ~1/4 of the full FOV?
@kecnry – what's the least disruptive way to implement footprint multiselect?
I think we had discussed this a while back, but can't remember the details. What functionality do you want to support with multiselect? I think the closest analog would be the implementation for subsets in subset tools - in multiselect mode the user is no longer allowed to view/edit the properties of the subset and only see actions that can be completed in bulk (currently recentering). |
Yes, that’s the behavior I implemented. The selection logic doesn’t look at the full FOV of the instrument, but instead compares the area of the specific mark polygons at the click location. So in your example, it’s picking NIRSpec because that individual polygon has the smallest area among those containing the click, even though the full FOV is larger. |

Description
This pull request:
Fixes #
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?