Mosviz redshift slider

[kecnry]

Description

This pull request is an extension of #961 and adds a redshift slider in Mosviz along with new mosviz.get_spectrum_1d(row=None) and mosviz.get_spectrum_2d(row=None) (defaults to the currently selected row if row is not provided) helper methods to access the underlying data products with their respective redshifts applied.

Note: support for RVs in the slider dropdown is supported, but roundtripping is not perfect. Support for an RV column in the table connected to the slider is not (currently) included.

This also currently includes #980 (which would preferably be merged first and then this PR rebased onto main)

Wishlist:

• fix "roundtripping" between redshift and RV (if possible)
  • roundtripping internally is confirmed to be fine. Issue is that the step of the slider itself is forcing values to round when switching between redshift and RV. Fixing this would require a redesign of how the bounds/step of the slider are determined. Determined to be out-of-scope (same issue existed before for specviz, just wasn't apparent since the redshift value isn't shown and it generally "rounds back" to the original value when re-selecting redshift) and filed as redshift slider roundtripping #987.
• investigate behavior for hidden redshift column when changing slider (any changed data will automatically force that column to be shown which conflicts with the hide_column logic).
  • postponing to wait for user feedback (some voted that the current behavior might actually be preferred). Changing this will likely require storing the desired list internally and re-setting the visible columns, or overriding the behavior internal to glue.
• if RV column is within scope: design and implement
• try to minimize/remove redshift column "flickering" when changing rows. (Now fairly minimal but not gone. Fixing this completely is likely a larger fix that can also cover the flickering in the viewers when changing rows and is likely out of scope)
• add test coverage. (Some done, but might be able to be improved before merge)
• merge refactor mosviz row lock to be plugin #980 and rebase

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

Merging #982 (f8cf637) into main (ede8d3b) will increase coverage by 2.14%.
The diff coverage is 92.39%.

@@            Coverage Diff             @@
##             main     #982      +/-   ##
==========================================
+ Coverage   70.49%   72.63%   +2.14%     
==========================================
  Files          74       74              
  Lines        5422     5576     +154     
==========================================
+ Hits         3822     4050     +228     
+ Misses       1600     1526      -74     

Impacted Files	Coverage Δ
...specviz/plugins/redshift_slider/redshift_slider.py	62.58% <33.33%> (+20.67%)	⬆️
jdaviz/configs/mosviz/helper.py	78.73% <93.33%> (+3.54%)	⬆️
jdaviz/configs/mosviz/plugins/viewers.py	81.20% <100.00%> (+0.51%)	⬆️
jdaviz/configs/specviz/helper.py	55.76% <100.00%> (+6.27%)	⬆️
...default/plugins/gaussian_smooth/gaussian_smooth.py	82.92% <0.00%> (-2.59%)	⬇️
...daviz/configs/default/plugins/collapse/collapse.py	79.81% <0.00%> (-0.39%)	⬇️
jdaviz/app.py	87.95% <0.00%> (-0.11%)	⬇️
...configs/cubeviz/plugins/moment_maps/moment_maps.py	58.82% <0.00%> (+0.05%)	⬆️
jdaviz/configs/mosviz/plugins/parsers.py	78.71% <0.00%> (+0.28%)	⬆️
... and 10 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ede8d3b...f8cf637. Read the comment docs.

[pllim]

Seems to work but can be improved with the following:

1. Add some user-facing documentation on the new UI improvement and also the two new public methods mentioned in change log.
2. When calling mosviz.get_spectrum_1d() and mosviz.get_spectrum_2d(), I see a warning that says WARNING:root:Warning: Applying the value from the redshift slider to the output spectra. To avoid seeing this warning, explicitly set the apply_slider_redshift argument to True or False., which is very confusing because I am calling the methods using default parameters, which are usually the recommended ones. Suggest change the default into something that does not produce a warning (unless this is what PO wanted...).
3. Is blueshift not supported at all? Don't some lines get blueshifted in some scientific situations?

[kecnry]

1. Add some user-facing documentation on the new UI improvement and also the two new public methods mentioned in change log.

Added basic information (and link to the Specviz redshift docs) in 8470c01. Let me know if you think anything is missing that needs to be there.

2. When calling mosviz.get_spectrum_1d() and mosviz.get_spectrum_2d(), I see a warning that says WARNING:root:Warning: Applying the value from the redshift slider to the output spectra. To avoid seeing this warning, explicitly set the apply_slider_redshift argument to True or False., which is very confusing because I am calling the methods using default parameters, which are usually the recommended ones. Suggest change the default into something that does not produce a warning (unless this is what PO wanted...).

This is to mirror the behavior of specviz.get_spectra() and was added in 8b8f2b0 by @rosteen but I don't know the motivation for that decision (edit: see #380 (comment)). If we make any changes, I think they should be made in both cases for consistency.

3. Is blueshift not supported at all? Don't some lines get blueshifted in some scientific situations?

Blueshift should be supported, but will need to currently be entered manually as otherwise the lower limit of the slider is always zero/positive. This case should be a consideration if/when redesigning the slider (relevant to #987).

[pllim]

I'll approve when tests are added. 😉

[rosteen]

Looks good, everything seems to be behaving and performing well. The only thing I think might trip some people up is if they wanted to set the redshift for all the rows - it might be nice to have an option to set row='all' in the mosviz.update_column call to do that without needing to construct an array of the correct length. But I don't think that should hold up the PR, so I'm going to approve and if you want to implement that now I can take another look before merging.

[rosteen]

@pllim There are tests now, do you approve? 😁

[pllim]

Thanks!