Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: Fix wrong Subset unit when created in 2D spectrum viewer #3201

Merged
merged 6 commits into from
Oct 1, 2024
Merged

BUG: Fix wrong Subset unit when created in 2D spectrum viewer #3201

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b1a3031
BUG: Fix wrong Subset unit in Specviz2d
pllim Sep 24, 2024
9dc6d6e
Add test and change log
pllim Sep 24, 2024
fe250fa
cubeviz is so special
pllim Sep 24, 2024
e4d6311
Address comment from kecnry
pllim Sep 30, 2024
3e501c0
Merge branch 'main' into specviz2d-subset-unit
pllim Sep 30, 2024
eafb391
Move change log to match new milestone
pllim Oct 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions CHANGES.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,6 +162,8 @@ Specviz
Specviz2d
^^^^^^^^^

- Fixed Subset unit when it is created in 2D spectrum viewer. [#3201]

Other Changes and Additions
---------------------------

Expand Down
25 changes: 18 additions & 7 deletions jdaviz/app.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@
from ipysplitpanes import SplitPanes
import matplotlib.cm as cm
import numpy as np
from glue.config import colormaps, settings as glue_settings
from glue.config import colormaps, data_translator, settings as glue_settings
from glue.core import HubListener
from glue.core.link_helpers import LinkSame, LinkSameWithUnits
from glue.core.message import (DataCollectionAddMessage,
Expand Down Expand Up @@ -1052,13 +1052,24 @@
simplify_spectral=True, use_display_units=False):
# TODO: Use global display units
# units = dc[0].data.coords.spectral_axis.unit
viewer = self.get_viewer(self._jdaviz_helper. _default_spectrum_viewer_reference_name)
data = viewer.data()
if data and len(data) > 0 and isinstance(data[0], Spectrum1D):
units = data[0].spectral_axis.unit
if not hasattr(subset_state.att, "parent"): # e.g., Cubeviz
viewer = self.get_viewer(self._jdaviz_helper._default_spectrum_viewer_reference_name)
data = viewer.data()
if data and len(data) > 0 and isinstance(data[0], Spectrum1D):
units = data[0].spectral_axis.unit

Check warning on line 1059 in jdaviz/app.py

View check run for this annotation

Codecov / codecov/patch

jdaviz/app.py#L1056-L1059 

Added lines #L1056 - L1059 were not covered by tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what does data[0] assume? That that is the full cube extracted spectrum? If that is the assumption, maybe we need to be more explicit and request it by label instead? What if that has been unloaded from the viewer?

I still wish this could be generalized, but I see how its difficult since data in the else is the glue component, but data in the if is a Spectrum1D (or other translated object). Although the else in the else then uses a handler to convert to a Spectrum1D anyways. 🤔

If all we need here are the units of the x-axis of the spectrum viewer... can we just access the display units? Or do we need the native units of the "reference" data?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know the answers. This block is really for Cubeviz and I am trying to patch Specviz2D. Do I really have to fix Cubeviz too?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair, ignore my first line of questions then since those do apply to the old code. But I think if we can have a general block here that applies to cubeviz and specviz2d, that would be easier to maintain than if-statements, right? There is even a TODO comment immediately above suggesting switching to display units - so maybe we can simplify this all down to one line once @gibsongreen enables display units across all configs?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(or maybe it makes sense to get this in like this now and either roll that into @gibsongreen's WIP or as a tech-debt follow-up after that makes it in as well?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, we have decided to revisit this after unit conversion is enabled across all configs and merge the logic as-is until then. 🐱

else:
raise ValueError("Unable to find spectral axis units")

Check warning on line 1061 in jdaviz/app.py

View check run for this annotation

Codecov / codecov/patch

jdaviz/app.py#L1061 

Added line #L1061 was not covered by tests
else:
raise ValueError("Unable to find spectral axis units")
if use_display_units:
data = subset_state.att.parent
ndim = data.get_component("flux").ndim
if ndim == 2:
units = u.pix
else:
handler, _ = data_translator.get_handler_for(Spectrum1D)
spec = handler.to_object(data)
units = spec.spectral_axis.unit

if use_display_units and units != u.pix:
# converting may result in flipping order (wavelength <-> frequency)
ret_units = self._get_display_unit('spectral')
subset_bounds = [(subset_state.lo * units).to(ret_units, u.spectral()),
Expand Down
22 changes: 22 additions & 0 deletions jdaviz/configs/specviz2d/tests/test_parsers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,10 @@
import pytest
import stdatamodels
from astropy import units as u
from astropy.io import fits
from astropy.utils.data import download_file
from glue.core.edit_subset_mode import NewMode
from glue.core.roi import XRangeROI

from jdaviz.utils import PRIHDR_KEY
from jdaviz.configs.imviz.tests.utils import create_example_gwcs
Expand Down Expand Up @@ -134,6 +137,25 @@ def test_2d_1d_parser(specviz2d_helper, mos_spectrum2d, spectrum1d):
specviz2d_helper.load_data(spectrum_2d=mos_spectrum2d, spectrum_1d=spectrum1d)
assert specviz2d_helper.app.data_collection.labels == ['Spectrum 2D', 'Spectrum 1D']

spec2d_viewer = specviz2d_helper.app.get_viewer("spectrum-2d-viewer")
assert spec2d_viewer.figure.axes[0].label == "x: pixels" # -0.5, 14.5
spec2d_viewer.apply_roi(XRangeROI(10, 12))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could have used the API from #3104 but since it is not merged yet, I didn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#3104 is now merged but the API is not yet public. Should I keep this as-is or should I use the helper.plugins['Subset Tools']._obj.import_region(...) syntax here?


spec2d_viewer.session.edit_subset_mode._mode = NewMode

spec1d_viewer = specviz2d_helper.app.get_viewer("spectrum-viewer")
assert spec1d_viewer.figure.axes[0].label == "Wavelength [Angstrom]" # 6000, 8000
spec1d_viewer.apply_roi(XRangeROI(7000, 7100))

# Subset 1 should follow Spectrum 2D viewer unit.
# Subset 2 should follow Spectrum 1D viewer unit.
subsets = specviz2d_helper.app.get_subsets()
assert len(subsets) == 2
s1 = subsets["Subset 1"]
s2 = subsets["Subset 2"]
assert s1.lower.unit == s1.upper.unit == u.pix
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I confirmed that this line throws exception on main without this patch, but passes with this patch. Exception on main:

>       assert s1.lower.unit == s1.upper.unit == u.pix
E       assert Unit("Angstrom") == Unit("pix")
E        +  where Unit("Angstrom") = <Quantity 12. Angstrom>.unit
E        +    where <Quantity 12. Angstrom> = Spectral Region, 1 sub-regions:\n  (10.0 Angstrom, 12.0 Angstrom) .upper
E        +  and   Unit("pix") = u.pix

assert s2.lower.unit == s2.upper.unit == u.AA


def test_parser_no_data(specviz2d_helper):
with pytest.raises(ValueError, match='Must provide spectrum_2d or spectrum_1d'):
Expand Down