loading line lists at redshifted wavelength #2726

gibsongreen · 2024-02-23T16:35:41Z

Description

This pull request is to address the following ticket:
https://jira.stsci.edu/browse/JDAT-2780

Where observed wavelengths are showing the correct shifted value but are being plotted at rest wavelengths until there is some interaction with the redshift in which case they immediately update to the correct position.

Pre-PR Behavior:
https://github.com/spacetelescope/jdaviz/assets/42986583/c690234c-85b6-442d-92b4-5c3010132c29

Post-PR Behavior:
https://github.com/spacetelescope/jdaviz/assets/42986583/636e517e-87f6-4576-adcf-b1c0bda1cc59

Fixes #

Change log entry

Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
should add a no-changelog-entry-needed label.

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 milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone. Bugfix milestone also needs an accompanying backport label.
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

gibsongreen · 2024-02-23T16:43:50Z

jdaviz/configs/default/plugins/line_lists/tests/test_line_lists.py

+    # Load remaining lines
+    specviz_helper.plot_spectral_lines(global_redshift)
+
+    viewer_lines = [mark for mark in specviz_helper.app.get_viewer(


If anyone has a suggestion to get around using a loop here and in the second test. It is here to get the SpectralLines in the viewer, and make sure the redshift is applied. The Qtable doesn't update automatically if redshift is applied so I went with this methodology for now.

@kecnry might know.

gibsongreen · 2024-02-23T16:45:39Z

jdaviz/configs/default/plugins/line_lists/line_lists.py

@@ -664,7 +665,8 @@ def vue_show_all_in_list(self, listname):
        self.list_contents = lc
        self.send_state('list_contents')

-        self._viewer.plot_spectral_lines()
+        self._viewer.plot_spectral_lines(global_redshift=self._global_redshift


This conditional also may be removed and it could just be global_redshift. It will either be 0 or whatever the value was updated to in the GUI.

and in the subsequent locations where plot_spectral_line and plot_spectral_lines is called

Having a global_redshift to be actually global app-wide would make more sense then passing it in like this, no? But I didn't design this plugin, so maybe other devs can chime in here.

We are redesigning line shifts in the near future to have this more clearly defined, so I think as a bugfix this is probably fine for now, but we probably should check to make sure this doesn't break any known notebooks that may try advanced (non-advertized) functionality for per-line or per-list redshifts.

codecov · 2024-02-23T16:51:05Z

Codecov Report

Attention: Patch coverage is 71.42857% with 4 lines in your changes are missing coverage. Please review.

Project coverage is 88.08%. Comparing base (a50aca4) to head (d859265).
Report is 4 commits behind head on main.

❗ Current head d859265 differs from pull request most recent head 8bc2d0d. Consider uploading reports for the commit 8bc2d0d to get more accurate results

Files	Patch %	Lines
...z/configs/default/plugins/line_lists/line_lists.py	25.00%	3 Missing ⚠️
jdaviz/configs/specviz/plugins/viewers.py	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2726      +/-   ##
==========================================
+ Coverage   88.07%   88.08%   +0.01%     
==========================================
  Files         108      108              
  Lines       15883    15889       +6     
==========================================
+ Hits        13989    13996       +7     
+ Misses       1894     1893       -1

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

pllim · 2024-02-24T02:01:46Z

jdaviz/configs/default/plugins/line_lists/line_lists.py

@@ -524,7 +524,8 @@ def _list_from_notebook(self, msg):
        self.send_state('loaded_lists')
        self.send_state('list_contents')

-        self._viewer.plot_spectral_lines(tmp_names_rest)
+        self._viewer.plot_spectral_lines(tmp_names_rest, global_redshift=self._global_redshift
+                                         if self._global_redshift != 0 else None)


Since this is floating point comparison, would allclose be safer?

Same comment applies to all similar occurrences.

pllim · 2024-02-24T02:02:55Z

jdaviz/configs/default/plugins/line_lists/tests/test_line_lists.py

+    spec = Spectrum1D(flux=np.random.rand(100)*u.Jy,
+                      spectral_axis=np.arange(6000, 7000, 10)*u.AA)
+    specviz_helper.load_data(spec)


No need to generate your own test data, especially since you already passing in the fixture.

Suggested change

spec = Spectrum1D(flux=np.random.rand(100)*u.Jy,

spectral_axis=np.arange(6000, 7000, 10)*u.AA)

specviz_helper.load_data(spec)

specviz_helper.load_data(spectrum1d)

Both tests have been updated to use the fixture data

pllim · 2024-02-24T02:06:23Z

jdaviz/configs/default/plugins/line_lists/tests/test_line_lists.py

+    lt = QTable()
+    lt['linename'] = ['O III', 'Halpha']
+    lt['rest'] = [5000., 6000.]*u.AA
+    lt['redshift'] = u.Quantity([0.0, 0.0])


You can shorten thus to one line. Also it bothers me the wavelengths are incorrect.

Suggested change

lt = QTable()

lt['linename'] = ['O III', 'Halpha']

lt['rest'] = [5000., 6000.]*u.AA

lt['redshift'] = u.Quantity([0.0, 0.0])

lt = QTable({'linename': ['O III', 'Halpha'], 'rest': [5007, 6563] * u.AA, 'redshift': u.Quantity([0, 0])})

The two new tests are now shortened to one line for Qtables, I held off on updating the other tests in line_list, which use multiple lines to create the table

pllim · 2024-02-24T02:12:04Z

jdaviz/configs/default/plugins/line_lists/line_lists.py

@@ -664,7 +665,8 @@ def vue_show_all_in_list(self, listname):
        self.list_contents = lc
        self.send_state('list_contents')

-        self._viewer.plot_spectral_lines()
+        self._viewer.plot_spectral_lines(global_redshift=self._global_redshift


Having a global_redshift to be actually global app-wide would make more sense then passing it in like this, no? But I didn't design this plugin, so maybe other devs can chime in here.

pllim · 2024-02-24T02:12:28Z

jdaviz/configs/default/plugins/line_lists/tests/test_line_lists.py

+    assert viewer_lines[0].redshift == 0.01
+    assert viewer_lines[1].redshift == 0.01


Since this is floating point comparison, would allclose be safer?

pllim · 2024-02-24T02:12:53Z

jdaviz/configs/default/plugins/line_lists/tests/test_line_lists.py

+    spec = Spectrum1D(flux=np.random.rand(100)*u.Jy,
+                      spectral_axis=np.arange(6000, 7000, 10)*u.AA)
+    specviz_helper.load_data(spec)


Suggested change

spec = Spectrum1D(flux=np.random.rand(100)*u.Jy,

spectral_axis=np.arange(6000, 7000, 10)*u.AA)

specviz_helper.load_data(spec)

specviz_helper.load_data(spectrum1d)

pllim · 2024-02-24T02:14:34Z

jdaviz/configs/default/plugins/line_lists/tests/test_line_lists.py

+    lt = QTable()
+    lt['linename'] = ['O III', 'Halpha', 'O III']
+    lt['rest'] = [5000., 6000., 7000.]*u.AA
+    lt['redshift'] = u.Quantity([0.0, 0.0, 0.0])


Suggested change

lt = QTable()

lt['linename'] = ['O III', 'Halpha', 'O III']

lt['rest'] = [5000., 6000., 7000.]*u.AA

lt['redshift'] = u.Quantity([0.0, 0.0, 0.0])

lt = QTable({'linename': ['O III', 'Halpha', 'O III'], 'rest': [5007, 6563, 4959] * u.AA, 'redshift': u.Quantity([0, 0, 0])})

pllim · 2024-02-24T02:15:09Z

jdaviz/configs/default/plugins/line_lists/tests/test_line_lists.py

+    # Load remaining lines
+    specviz_helper.plot_spectral_lines(global_redshift)
+
+    viewer_lines = [mark for mark in specviz_helper.app.get_viewer(


@kecnry might know.

pllim · 2024-02-24T02:15:31Z

jdaviz/configs/default/plugins/line_lists/tests/test_line_lists.py

+    assert viewer_lines[0].redshift == 0.01
+    assert viewer_lines[1].redshift == 0.01
+    assert viewer_lines[2].redshift == 0.01


Since this is floating point comparison, would allclose be safer?

All the asserts are now updated to use allclose

jdaviz/configs/specviz/plugins/viewers.py

pllim

Since plugin is going to be refactored anyway, this is good enough. Thanks!

@kecnry , do you have certain notebooks in mind, as per your comment above?

pllim · 2024-02-26T17:42:42Z

Still need a change log under v3.8.3 section for bug fix. FYI.

kecnry

Works for me for now! We'll obviously need to re-think some of this when we redesign to break global redshift assumption, but we have the go ahead to ignore that for now. Thanks!

gibsongreen · 2024-02-26T17:56:52Z

Still need a change log under v3.8.3 section for bug fix. FYI.

This is a simple question that I should've asked a while ago, is there a way to know which version a PR belongs to?

kecnry · 2024-02-26T18:01:04Z

The milestone should be set, and if it is to a bugfix, then the backport bugfix label must be added manually.

…new line being added to viewer

…r: more readable if; line_list: cut conditional logic of redshift

kecnry · 2024-02-26T19:17:49Z

CHANGES.rst

+- Fix redshifted line lists that were displaying at rest wavelengths. Global redshift
+  used, this will be refactored in the future when setting redshift per-line or per-list. [#2726]


this might be confusing from the user-perspective, maybe this is sufficient?

Suggested change

- Fix redshifted line lists that were displaying at rest wavelengths. Global redshift

used, this will be refactored in the future when setting redshift per-line or per-list. [#2726]

- Fix redshifted line lists that were displaying at rest wavelengths, by assuming a global redshift. [#2726]

Got it, thanks!

* updating lines so they receive redshift if redshift applied prior to new line being added to viewer * test: cut/simplied logic and excess data, allclose for asserts; viewer: more readable if; line_list: cut conditional logic of redshift * added change log * change log text altered (cherry picked from commit 128d642)

gibsongreen added bug Something isn't working specviz mosviz plugin Label for plugins common to multiple configurations specviz2d labels Feb 23, 2024

gibsongreen requested review from rosteen, javerbukh, pllim, kecnry, bmorris3, haticekaratay and cshanahan1 as code owners February 23, 2024 16:35

gibsongreen commented Feb 23, 2024

View reviewed changes

pllim reviewed Feb 24, 2024

View reviewed changes

pllim added this to the 3.8.3 milestone Feb 26, 2024

pllim added the 💤backport-v3.8.x on-merge: backport to v3.8.x label Feb 26, 2024

pllim approved these changes Feb 26, 2024

View reviewed changes

kecnry approved these changes Feb 26, 2024

View reviewed changes

gibsongreen added 3 commits February 26, 2024 13:35

updating lines so they receive redshift if redshift applied prior to …

1a3e16b

…new line being added to viewer

test: cut/simplied logic and excess data, allclose for asserts; viewe…

8f2ddfc

…r: more readable if; line_list: cut conditional logic of redshift

added change log

7512c20

gibsongreen force-pushed the line_list_bug branch from d859265 to 7512c20 Compare February 26, 2024 19:08

kecnry reviewed Feb 26, 2024

View reviewed changes

change log text altered

8bc2d0d

gibsongreen removed the specviz label Feb 26, 2024

gibsongreen added the cubeviz label Feb 26, 2024

gibsongreen merged commit 128d642 into spacetelescope:main Feb 26, 2024
18 checks passed

This comment was marked as resolved.

Sign in to view

lumberbot-app bot added the Still Needs Manual Backport label Feb 26, 2024

gibsongreen mentioned this pull request Feb 26, 2024

Backport PR #2726 on branch v3.8.x (loading line lists at redshifted wavelength) #2728

Merged

9 tasks

gibsongreen added no-changelog-entry-needed changelog bot directive trivial Only needs one approval instead of two and removed trivial Only needs one approval instead of two no-changelog-entry-needed changelog bot directive labels Feb 26, 2024

pllim removed the Still Needs Manual Backport label Feb 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

loading line lists at redshifted wavelength #2726

loading line lists at redshifted wavelength #2726

gibsongreen commented Feb 23, 2024 •

edited

Loading

gibsongreen Feb 23, 2024

pllim Feb 24, 2024

gibsongreen Feb 23, 2024

gibsongreen Feb 23, 2024 •

edited

Loading

pllim Feb 24, 2024

kecnry Feb 26, 2024

codecov bot commented Feb 23, 2024 •

edited

Loading

pllim Feb 24, 2024

pllim Feb 24, 2024

gibsongreen Feb 26, 2024

pllim Feb 24, 2024

gibsongreen Feb 26, 2024

pllim Feb 24, 2024

pllim Feb 24, 2024

pllim Feb 24, 2024

pllim Feb 24, 2024

pllim Feb 24, 2024

pllim Feb 24, 2024

gibsongreen Feb 26, 2024

pllim left a comment

pllim commented Feb 26, 2024

kecnry left a comment

gibsongreen commented Feb 26, 2024

kecnry commented Feb 26, 2024

kecnry Feb 26, 2024

gibsongreen Feb 26, 2024

This comment was marked as resolved.

		assert viewer_lines[0].redshift == 0.01
		assert viewer_lines[1].redshift == 0.01

		- Fix redshifted line lists that were displaying at rest wavelengths. Global redshift
		used, this will be refactored in the future when setting redshift per-line or per-list. [#2726]

loading line lists at redshifted wavelength #2726

loading line lists at redshifted wavelength #2726

Conversation

gibsongreen commented Feb 23, 2024 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibsongreen Feb 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 23, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pllim left a comment

Choose a reason for hiding this comment

pllim commented Feb 26, 2024

kecnry left a comment

Choose a reason for hiding this comment

gibsongreen commented Feb 26, 2024

kecnry commented Feb 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

gibsongreen commented Feb 23, 2024 •

edited

Loading

gibsongreen Feb 23, 2024 •

edited

Loading

codecov bot commented Feb 23, 2024 •

edited

Loading