prevent spectral lines from converting y-range #3242

gibsongreen · 2024-10-22T15:09:53Z

Description

This pull request is to address line lists and unit conversion compatibility. On main, line marks go through conversion machinery and can cause lines to vanish from the spectrum-viewer depending on the original unit and target unit (are not visible even at updated y-range for line after conversion). This PR prevents SpectralLines from y-converting so they maintain their position in the viewer at the rest + redshifted spectral value.

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)?

jdaviz/core/marks.py

codecov · 2024-10-22T15:32:19Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.63%. Comparing base (f552510) to head (ea224d9).

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #3242   +/-   ##
=======================================
  Coverage   88.63%   88.63%           
=======================================
  Files         125      125           
  Lines       18779    18782    +3     
=======================================
+ Hits        16644    16647    +3     
  Misses       2135     2135

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

jdaviz/core/marks.py

pllim · 2024-10-22T17:10:25Z

jdaviz/core/marks.py

@@ -244,6 +244,11 @@ def set_x_unit(self, unit=None):
        super().set_x_unit(unit=unit)
        self._rest_value = (self._rest_value * prev_unit).to_value(unit, u.spectral())

+    def set_y_unit(self, unit=None):
+        # prevent spectral lines from entering unit conversion machinery so they maintain
+        # their position in the viewer at the rest + redshifted spectral value


Is a regression test possible? I guess you can check marker values? 🤷‍♀️

Does this need a bug fix change log?

For a regression test, the y-value range of the mark for the line will always be [0, 1] with this change so I don't think it is possible here.

There was a mention of backporting in tagup for this PR, I think it would be for v4.0.x but wasn't certain. I will add it 4.0.1 change log now.

do you think it would be possible for PluginMark._on_global_display_unit_changed to check for LinearScale(0, 1) and skip the call to getattr(self, f'set_{axis}_unit')(msg.unit) if so? That way this can be applied in general across all marks (it looks now like the slice indicator handles this manually by overriding _on_global_display_unit_changed - maybe that could be removed as well then).

Functionality was maintained with this change and the update to no longer override for the slice indicator also works!

kecnry · 2024-10-22T18:01:50Z

jdaviz/core/marks.py

-        # Updating the value is handled by the plugin itself, need to update unit string.
-        if msg.axis in ["spectral", "x"]:
-            self.xunit = msg.unit
-        self._update_label()


the _update_label may still be necessary to change the units in the label, but the rest probably can now be replaced with super()._on_global_display_unit_changed(msg). Sorry for missing that!

I'll double check this now

I'm seeing that the values for LinearScale and the slice indicator are not (0, 1). I can alter it to just test for LinearScale but I will need to check if there are an adverse affects from this.

If I do alter this and include this change, the label value updates but the unit does not on the slice indicator (which should be feasible to update but is proving to be a bit tricky).

A note that if the _global_display_unit_changed is not altered for SliceIndicator, there is no need for the LinearScale in this comment and everything functions as expected.

…rScale(0,1), remove slice override to also use it

…ng y-range

) Co-authored-by: Gilbert Green <42986583+gibsongreen@users.noreply.github.com>

gibsongreen added cubeviz specviz trivial Only needs one approval instead of two plugin Label for plugins common to multiple configurations labels Oct 22, 2024

gibsongreen requested review from rosteen, javerbukh, pllim, kecnry, bmorris3, haticekaratay and cshanahan1 as code owners October 22, 2024 15:09

pllim reviewed Oct 22, 2024

View reviewed changes

jdaviz/core/marks.py Outdated Show resolved Hide resolved

pllim reviewed Oct 22, 2024

View reviewed changes

jdaviz/core/marks.py Outdated Show resolved Hide resolved

pllim reviewed Oct 22, 2024

View reviewed changes

pllim added this to the 4.0.1 milestone Oct 22, 2024

pllim added the 💤backport-v4.0.x on-merge: backport to v4.0.x label Oct 22, 2024

kecnry reviewed Oct 22, 2024

View reviewed changes

kecnry approved these changes Oct 25, 2024

View reviewed changes

cshanahan1 approved these changes Oct 30, 2024

View reviewed changes

gibsongreen added 6 commits October 30, 2024 10:17

prevent spectral lines for reseting y value/unit

c06fadf

override set_y_unit for SpectralLine

fb04851

add comment

4143cb2

add change log

97bd922

generalize preventing PluginMarks from converting y values when Linea…

ac863bb

…rScale(0,1), remove slice override to also use it

revert SliceIndicator _global_display_unit_changed

e7190ae

gibsongreen force-pushed the prevent-spec-line-y-conversion branch from ea224d9 to e7190ae Compare October 30, 2024 14:19

gibsongreen enabled auto-merge (squash) October 31, 2024 14:54

gibsongreen disabled auto-merge October 31, 2024 14:55

kecnry merged commit 927f84d into spacetelescope:main Nov 1, 2024
15 of 17 checks passed

meeseeksmachine pushed a commit to meeseeksmachine/jdaviz that referenced this pull request Nov 1, 2024

Backport PR spacetelescope#3242: prevent spectral lines from converti…

963d297

…ng y-range

meeseeksmachine mentioned this pull request Nov 1, 2024

Backport PR #3242 on branch v4.0.x (prevent spectral lines from converting y-range) #3266

Merged

kecnry pushed a commit that referenced this pull request Nov 1, 2024

Backport PR #3242: prevent spectral lines from converting y-range (#3266

64cb2c2

) Co-authored-by: Gilbert Green <42986583+gibsongreen@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

prevent spectral lines from converting y-range #3242

prevent spectral lines from converting y-range #3242

gibsongreen commented Oct 22, 2024 •

edited

Loading

codecov bot commented Oct 22, 2024 •

edited

Loading

pllim Oct 22, 2024

gibsongreen Oct 22, 2024

kecnry Oct 22, 2024

gibsongreen Oct 22, 2024

kecnry Oct 22, 2024

gibsongreen Oct 22, 2024

gibsongreen Oct 22, 2024 •

edited

Loading

prevent spectral lines from converting y-range #3242

prevent spectral lines from converting y-range #3242

Conversation

gibsongreen commented Oct 22, 2024 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

codecov bot commented Oct 22, 2024 • edited Loading

Codecov Report

pllim Oct 22, 2024

Choose a reason for hiding this comment

gibsongreen Oct 22, 2024

Choose a reason for hiding this comment

kecnry Oct 22, 2024

Choose a reason for hiding this comment

gibsongreen Oct 22, 2024

Choose a reason for hiding this comment

kecnry Oct 22, 2024

Choose a reason for hiding this comment

gibsongreen Oct 22, 2024

Choose a reason for hiding this comment

gibsongreen Oct 22, 2024 • edited Loading

Choose a reason for hiding this comment

gibsongreen commented Oct 22, 2024 •

edited

Loading

codecov bot commented Oct 22, 2024 •

edited

Loading

gibsongreen Oct 22, 2024 •

edited

Loading