Interactive line identification #1115
Conversation
kecnry
force-pushed
the
line-identify
branch
from
413b604 to
f51d324
Compare
Codecov Report
@@ Coverage Diff @@
## main #1115 +/- ##
==========================================
+ Coverage 76.37% 76.85% +0.47%
==========================================
Files 87 87
Lines 6723 6935 +212
==========================================
+ Hits 5135 5330 +195
- Misses 1588 1605 +17
Continue to review full report at Codecov.
|
kecnry
force-pushed
the
line-identify
branch
2 times, most recently
from
524ff5e to
235544f
Compare
kecnry
force-pushed
the
line-identify
branch
6 times, most recently
from
a9322ee to
c731e21
Compare
| lt['redshift'] = u.Quantity(0.046) | ||
| with pytest.warns(UserWarning, match='per line/list redshifts not supported, use viz.set_redshift'): # noqa | ||
| specviz_helper.load_line_list(lt) |
There was a problem hiding this comment.
already covered in test_redshift test, no sense in repeating it here.
kecnry
requested review from
duytnguyendtn,
rosteen,
javerbukh,
ibusko,
ojustino and
pllim
as code owners
| # update the RV and all other obs wavelengths. Once tabbing or losing focus, vue will | ||
| # send another event with avoid_feedback=False so that the wavelength updates to | ||
| # exactly match the redshift (so that can be considered the ground truth value consistently) | ||
| if kwargs.get('avoid_feedback', False): |
There was a problem hiding this comment.
Maybe store kwargs.get('avoid_feedback', False) in a var, since it is called again below?
* and start work on some optimizing based on plugin being opened.... still need to try to only updated obs values if the accordion for that list is opened.
* was resulting in a feedback loop between the two instances causing laggy drag as well as confusion between SelectSlice and SelectLine
* click un-identifies it
This was caused because the nested toolbar in #1140 was initialized twice for cubeviz's spectrum-viewer and there was a feedback loop between the two instances (which also resulted in very laggy dragging of the slice indicator... if for some reason the fix in this PR does not make the next release, this should probably be split out as a separate quick bugfix).
That's a good idea, added. Clicking the label is now also a quick way to clear the selection. |
|
Looks like test failure is the same as #1178 (comment) and can likely be ignored. |
|
A few comments based on my testing:
Screen.Recording.2022-03-18.at.1.44.03.PM.mov
I really like the redshift from centroid feature, it feels great to have the line lists more seamlessly integrated into other line-related functionality. |
|
@rosteen - I tried to address two of your comments (scrolling unfortunately isn't feasible), let me know what you think! I was trying to keep the behavior consistent so that it would always default to whatever was in that 4th menu entry, but I can see for now how that probably isn't ideal in the case of specviz. Down the road it might even be nice to hide tools that aren't usable (hide line selection until there are lines, etc), and then it might make more sense to re-enable that behavior. |
to avoid shifting in the plugin, but also to provide tooltips with help text
* results_centroid is now an internal trait which is observed to avoid multiple manual calls to updating the redshift value. This should ensure its always kept up to date.
|
Looks good to me now, thanks! I'm excited to get this in, it feels like it really bumps up the usefulness of the line lists by an order of magnitude. |
There was a problem hiding this comment.
Works great! No lag when loading all SDSS lines in Specviz but some lag in Cubeviz, but I don't think fixing that is in scope here.
Just one more minor question: I notice that this identify button is stacked with slice in Cubeviz, but it is standalone in Specviz. If that is intentional, then this is good to go!
Description
This pull request implements interactive support to help identify lines in line lists by highlighting a selected line, a new tool
(needs an icon)to click to select a line, as well as the ability to assign a redshift based on the centroid returned by the line analysis plugin and the selected line.This UX-pattern (ability to "lock" a dropdown with the selected item in the plot) could be extended in the future for subsets.
NOTE: icons have been updated since the following recording (see screenshot following) and the tool has been moved into the nested toolbar (available in the submenu for the slice indicator in cubeviz), but behavior has not changed.
Screen.Recording.2022-02-24.at.1.31.53.PM.mov
Updated with custom icon:
TODO:
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.CHANGES.rst?