move redshift slider to line list plugin

[kecnry]

Description

This pull request moves the redshift slider to the line list plugin (in the plugin tray), which allows viewing the redshift and RV simultaneously as well allowing a redesign of the slider functionality (which now acts as a delta-redshift slider) to fix the roundtripping issues seen in #987. This affects both specviz and mosviz (by removing the slider from the toolbar), and also adds redshift support to cubeviz.

API CHANGE: this also changes specviz.set_redshift_slider_bounds(lower, upper, step) to specviz.set_redshift_slider_bounds(range, step) and adds support for passing 'auto' to either range or step to then keep in sync with spectrum-viewer x-limits.

automatic slider limits (can be overridden with API, styling slightly out-of-date from screenshot above):

Screen.Recording.2022-01-06.at.4.30.06.PM.mov

TODO

• hook up existing Specviz helper API to set slider bounds/stepsize, while defaulting to width of spectrum viewer
• fix subset selection (seems to have broken for some reason, works in main)
• UI indication of range/units of slider (+/-dredshift)?
• update line list plugin docs, deprecate redshift docs
• test coverage (former redshift test coverage only applied to limits-logic, which no longer applies)
• changelog entry
• comments/changes from Jenn

FUTURE IDEAS (not necessarily in this PR)

• attempt to fix clicking (but not dragging slider). Might be an upstream bug that the mouseup event is getting blocked from bubbling up which I also ran into in Proof of concept: transparent menu while interacting with sliders #1021?
• should +/- limits under slider have indication they correspond to (unitless) redshift?
• fix [BUG]: xrange/box zoom breaks redshifting lines #1032 (exists in main, but still present here)
• RV unit dropdown (ability to view/set in units other than km/s)
• update line shifts without destroying and redrawing, handle y-scale change (see Slice Wavelength Indicator #1013 )
• ability to set observed wavelength of line and compute redshift (probably requires minor redesign of line table)
• consider use-case of setting redshift in mosviz without enabling lines
• custom lines unit dropdown or error checking (currently entering an invalid unit can break)
• (global) line indicator options similar to Slice Wavelength Indicator #1013 to show wavelength, name, etc next to lines

Fixes #987, fixes #473

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 change log needed? If yes, is it added to CHANGES.rst?
• Is a milestone set? Milestone is only currently required for PRs related to Imviz MVP.
• After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

[codecov]

Codecov Report

Attention: Patch coverage is 84.09091% with 21 lines in your changes missing coverage. Please review.

Project coverage is 73.70%. Comparing base (d7ae9bd) to head (3294442).
Report is 2744 commits behind head on main.

Files	Patch %	Lines
...z/configs/default/plugins/line_lists/line_lists.py	84.03%	19 Missing ⚠️
jdaviz/configs/specviz/plugins/viewers.py	77.77%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1031      +/-   ##
==========================================
+ Coverage   73.23%   73.70%   +0.46%     
==========================================
  Files          75       74       -1     
  Lines        5657     5620      -37     
==========================================
- Hits         4143     4142       -1     
+ Misses       1514     1478      -36     

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

[pllim]

Overall, good work thus far! Some comments below:

• While 42 is the answer for everything, setting the limits to +/-0.42 does seem pretty arbitrary. Why not +/-0.5?
• When I start at redshift=0 and I drag the slider all the way to -0.42, the final redshift does not end up being -0.42. I guess I'll wait for your documentation updates to see if this is expected or not.
• What is the status of round-tripping fix? When I copy paste the RV value at redshift=0.1 from the plugin, change the redshift to another value, and then paste that RV value back, the redshift displayed ends up being 0.10000000000000009. Is this expected?
• When I manually enter the redshift value, there is a noticeable lag for RV to update. I don't know if fixing this lag is out of scope or not. I have the Specviz example loaded and SDSS lines plotted.
• It is nice that blueshift is naturally supported now. 👍

[pllim]

RTD failure is real. docs/specviz/plugins.rst:140: WARNING: malformed hyperlink target.

[kecnry]

@pllim

• While 42 is the answer for everything, setting the limits to +/-0.42 does seem pretty arbitrary. Why not +/-0.5?

The limits of the delta-redshift slider default to a rough estimate of the redshift needed to move a line across the x-limits of the spectrum viewer... 0.42 is just a happy accident here. See the _auto_slider_range method for implementation details.

• When I start at redshift=0 and I drag the slider all the way to -0.42, the final redshift does not end up being -0.42. I guess I'll wait for your documentation updates to see if this is expected or not.

I cannot reproduce (it does go to -0.42 for me, although to +0.42 does show a precision issue at 0.420000001). Can you show a screenrecording? Regarding the precision "issue", I could always round the shown value to a number of decimal points, but I think that could have unexpected consequences and would need to be thought out carefully.

• What is the status of round-tripping fix? When I copy paste the RV value at redshift=0.1 from the plugin, change the redshift to another value, and then paste that RV value back, the redshift displayed ends up being 0.10000000000000009. Is this expected?

That's a good point, the roundtripping is only actually fixed with respect to the UI (i.e. you enter a redshift and that redshift is preserved and not slightly changed by internal recursive logic), but the actual equations in _redshift_to_velocity and _velocity_to_redshift are unchanged and still do not perfectly roundtrip. The stated redshift is what is actually applied to the lines.

• When I manually enter the redshift value, there is a noticeable lag for RV to update. I don't know if fixing this lag is out of scope or not. I have the Specviz example loaded and SDSS lines plotted.

Most of the lag is caused by replotting the lines and was a known "issue" (hopefully its almost immediate for a handful of lines), I have a plan to optimize this, but it's probably out of scope here and will be easier after #1013 is merged to avoid conflicts.

[pllim]

do not perfectly roundtrip

Should that be documented, if not already?

[rosteen]

I haven't looked into the code yet, but the first thing I noticed in testing is that this performs significantly worse than the slider on main. You can see in the first recording below that the slider is quite laggy, taking up to a couple seconds to catch up to where it should be when I let go of the slider. The second recording is the current state with the same number of lines plotted, which still flickers a little but is much more responsive.

Screen.Recording.2022-01-07.at.1.10.47.PM.mov

Screen.Recording.2022-01-07.at.1.12.56.PM.mov

[kecnry]

Should that be documented, if not already?

@pllim - that can never hurt, any suggestions where it best belongs?

the slider is quite laggy

@rosteen - I tried a few things which seem to help, but I can't reproduce quite as noticeable of a difference on my end. I do have ideas to optimize the line drawing (so that we scale better with Nlines but also to have the throttle timeout some function of the number of lines to prevent cases where the drawing lags steps behind), but those will be much easier to handle once this and #1013 are both merged. I agree that any up-front overhead that doesn't scale with Nlines should be minimized as much as possible here, though, to at least achieve roughly the same responsiveness as before.

[pllim]

any suggestions where it best belongs?

What about a new entry under https://github.com/spacetelescope/jdaviz/blob/main/docs/known_bugs.rst ?

[pllim]

See astropy/astropy#12709 . Maybe after astropy 5.1 is out, we can use that instead...

[kecnry]

sure! Although dropping unit conversion did help with some overhead since this is called on every slider update, so we will need to consider any time cost (which we'll need to do anyways if we want to support reading/setting RVs in other units)

[pllim]

Re: performance -- Is it possible to only convert the value when mouse release is detected on the slider (i.e., user stops sliding)?

[kecnry]

yes, that's an option, but I think its nice to see the RV updating live. (Note: we're no longer worried about performance as-is and have a working version outside this PR which will drastically improve lagginess).

[pllim]

If this conversion is not a true bottleneck for performance, might be better to use what is in astropy when that is available. That way, you know that functionality is vigorously tested upstream and it would natively support Quantity, so you can remove the comment about "if supporting non-km/s units in the future". Something to think about.

[rosteen]

I just pushed a few minor code changes (use isinstance instead of type() == in the line code, and a more detailed docstring). Everything else looks good to me, since there will be a performance PR to follow this up.

[pllim]

Should we reserve a second approval from PO?

[rosteen]

Should we reserve a second approval from PO?

I think that will be more relevant for the performance PR? We've already at least had verbal approval of this during meetings, I'm ok merging it with only dev approvals here.

[pllim]

Just nitpick on the code, otherwise LGTM but I want to withhold approval until I know I am looking at the final version of this PR, given the last TODO box is still unchecked, as per Slack discussion on 2022-01-11.

[pllim]

If this conversion is not a true bottleneck for performance, might be better to use what is in astropy when that is available. That way, you know that functionality is vigorously tested upstream and it would natively support Quantity, so you can remove the comment about "if supporting non-km/s units in the future". Something to think about.

[pllim]

Line list name not compatible with dark mode but I am not sure if fixing it is in scope or not, so I am approving because the slider itself works, with the knowledge that there will be follow-up PR for the performance, etc.

If fixing text color is not in scope here, I will open new issue.