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.

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

implement unit conversion in specviz2d #3253

Open
wants to merge 25 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
086cfa4
first pass specviz2d implementation
gibsongreen Oct 1, 2024
a14a39f
resolve stash
gibsongreen Oct 18, 2024
8032fdb
fix issues with stash and update get_data
gibsongreen Oct 18, 2024
35da355
second pass specviz2d uc implementation
gibsongreen Oct 21, 2024
b8eb091
revert 2D viewer for uc
gibsongreen Oct 21, 2024
34832f0
don't allow conversion of spectral line y values
gibsongreen Oct 22, 2024
c9ef4e2
fix bug to ensure non-scale factor data converts on _handle_display_u…
gibsongreen Oct 24, 2024
8ad5973
fix get_data bug using native unit for non-scale factor data
gibsongreen Oct 24, 2024
ef83da1
add line analysis uc test
gibsongreen Oct 24, 2024
6010858
add test for solid angle equivalency list
gibsongreen Oct 24, 2024
a06159a
add MosvizProfileView test
gibsongreen Oct 24, 2024
e8d22ac
add change log
gibsongreen Oct 24, 2024
eb45c00
reconcile test failures
gibsongreen Oct 24, 2024
37e7b6c
add API test
gibsongreen Oct 24, 2024
19a2868
remove global display unit change handler, syntax error in helpers
gibsongreen Oct 24, 2024
0a82322
ensure continuum marks don't double translate, test coverage
gibsongreen Oct 24, 2024
2f1db3e
move line analysis test
gibsongreen Oct 25, 2024
ea8cb25
remove files that weren't removed during rebase
gibsongreen Nov 29, 2024
ff6ac14
fix styling
gibsongreen Nov 29, 2024
f2920e2
add missing import
gibsongreen Dec 11, 2024
b876cb4
add spectral unit support, add eqvs to model fit, coords, marks
gibsongreen Dec 17, 2024
f946861
fix syntax error
gibsongreen Dec 17, 2024
7c54327
2dviewer spectral conversion, first iteration of test coverage
gibsongreen Dec 18, 2024
0ffdb5d
resolve test failures
gibsongreen Dec 19, 2024
98f2852
address spec density edge case, test coverage
gibsongreen Dec 19, 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
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,8 @@ Specviz
Specviz2d
^^^^^^^^^

- Implement the Unit Conversion plugin the Specviz2D. [#3253]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- Implement the Unit Conversion plugin the Specviz2D. [#3253]
- Implement the Unit Conversion plugin in Specviz2D. [#3253]


API Changes
-----------
- Removed API access to plugins that have passed the deprecation period: Links Control, Canvas Rotation, Export Plot. [#3270]
Expand Down
2 changes: 2 additions & 0 deletions jdaviz/configs/cubeviz/helper.py
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,8 @@ def get_data(self, data_label=None, spatial_subset=None, spectral_subset=None,
Spectral subset applied to data.
cls : `~specutils.Spectrum1D`, `~astropy.nddata.CCDData`, optional
The type that data will be returned as.
use_display_units : bool, optional
Specify whether the returned data is in native units or the current display units.

Returns
-------
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -520,9 +520,18 @@ def test_spectral_extraction_with_correct_sum_units(cubeviz_helper,
cubeviz_helper.load_data(spectrum1d_cube_fluxunit_jy_per_steradian)
spec_extr_plugin = cubeviz_helper.plugins['Spectral Extraction']._obj
collapsed = spec_extr_plugin.extract()

assert '_pixel_scale_factor' in collapsed.meta

# Original units in Jy / sr
expected_flux_values = [190., 590., 990., 1390., 1790., 2190., 2590., 2990., 3390., 3790.]
# After collapsing, sr is removed via the scale factor and the extracted spectrum is in Jy
expected_flux_values = [flux * collapsed.meta.get('_pixel_scale_factor')
for flux in expected_flux_values]
Comment on lines +527 to +530
Copy link
Member

Choose a reason for hiding this comment

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

can this be done with array math instead of a list comprehension?


np.testing.assert_allclose(
collapsed.flux.value,
[190., 590., 990., 1390., 1790., 2190., 2590., 2990., 3390., 3790.]
expected_flux_values
)
assert collapsed.flux.unit == u.Jy
assert collapsed.uncertainty.unit == u.Jy
Expand Down
7 changes: 7 additions & 0 deletions jdaviz/configs/default/plugins/markers/markers.py
Original file line number Diff line number Diff line change
Expand Up @@ -64,10 +64,12 @@ def __init__(self, *args, **kwargs):
elif self.config == 'specviz':
headers = ['spectral_axis', 'spectral_axis:unit',
'index', 'value', 'value:unit']

elif self.config == 'specviz2d':
# TODO: add "index" if/when specviz2d supports plotting spectral_axis
headers = ['spectral_axis', 'spectral_axis:unit',
'pixel_x', 'pixel_y', 'value', 'value:unit', 'viewer']

elif self.config == 'mosviz':
headers = ['spectral_axis', 'spectral_axis:unit',
'pixel_x', 'pixel_y', 'world_ra', 'world_dec', 'index',
Expand Down Expand Up @@ -223,6 +225,11 @@ def _on_is_active_changed(self, *args):
def _on_viewer_key_event(self, viewer, data):
if data['event'] == 'keydown' and data['key'] == 'm':
row_info = self.coords_info.as_dict()
if 'value' in row_info:
# format and cast flux value for Table
row_info['value'] = f"{row_info['value']:.5e}"
else:
row_info['value'] = ''
Comment on lines +228 to +232
Copy link
Member

Choose a reason for hiding this comment

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

why is this necessary? Can/should we move it to as_dict instead?


if 'viewer' in self.table.headers_avail:
row_info['viewer'] = viewer.reference if viewer.reference is not None else viewer.reference_id # noqa
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import numpy as np
from numpy.testing import assert_allclose
import pytest
from specutils import Spectrum1D

from jdaviz.core.custom_units_and_equivs import PIX2, SPEC_PHOTON_FLUX_DENSITY_UNITS
from jdaviz.core.marks import MarkersMark
Expand Down Expand Up @@ -242,6 +243,61 @@ def test_markers_cubeviz_flux_unit_conversion(cubeviz_helper,
assert last_row['value:unit'] == new_cube_unit_str


def test_markers_specviz2d_unit_conversion(specviz2d_helper, spectrum2d):
data = np.zeros((5, 10))
data[3] = np.arange(10)
spectrum2d = Spectrum1D(flux=data*u.MJy, spectral_axis=data[3]*u.AA)
specviz2d_helper.load_data(spectrum2d)

uc = specviz2d_helper.plugins['Unit Conversion']
uc.open_in_tray()
mp = specviz2d_helper.plugins['Markers']
mp.keep_active = True

label_mouseover = specviz2d_helper.app.session.application._tools["g-coords-info"]
viewer2d = specviz2d_helper.app.get_viewer("spectrum-2d-viewer")
label_mouseover._viewer_mouse_event(viewer2d, {"event": "mousemove",
"domain": {"x": 6, "y": 3}})
assert label_mouseover.as_text() == ('Pixel x=06.0 y=03.0 Value +6.00000e+00 MJy',
'Wave 6.00000e+00 Angstrom',
'')
mp._obj._on_viewer_key_event(viewer2d, {'event': 'keydown',
'key': 'm'})

# make sure last marker added to table reflects new unit selection
last_row = mp.export_table()[-1]
assert last_row['value:unit'] == uc.flux_unit
assert last_row['spectral_axis:unit'] == uc.spectral_unit

# ensure marks work with flux conversion where spectral axis is required and
# spectral axis conversion
uc.flux_unit = 'erg / (Angstrom s cm2)'
uc.spectral_unit = 'Ry'
label_mouseover._viewer_mouse_event(viewer2d, {"event": "mousemove",
"domain": {"x": 4, "y": 3}})
assert label_mouseover.as_text() == ('Pixel x=04.0 y=03.0 Value +7.49481e+00 erg / (Angstrom s cm2)', # noqa
'Wave 2.27817e+02 Ry',
'')
mp._obj._on_viewer_key_event(viewer2d, {'event': 'keydown',
'key': 'm'})

# make sure last marker added to table reflects new unit selection
last_row = mp.export_table()[-1]
assert last_row['value:unit'] == uc.flux_unit
assert last_row['spectral_axis:unit'] == uc.spectral_unit

second_marker_flux_unit = uc.flux_unit.selected
second_marker_spectral_unit = uc.spectral_unit.selected

# test edge case two non-native spectral axis required conversions
uc.flux_unit = 'ph / (Angstrom s cm2)'
uc.spectral_unit = 'eV'

# make sure table flux and spectral unit doesn't update
assert last_row['value:unit'] == second_marker_flux_unit
assert last_row['spectral_axis:unit'] == second_marker_spectral_unit


class TestImvizMultiLayer(BaseImviz_WCS_NoWCS):
def test_markers_layer_cycle(self):
label_mouseover = self.imviz.app.session.application._tools['g-coords-info']
Expand Down
12 changes: 11 additions & 1 deletion jdaviz/configs/default/plugins/model_fitting/model_fitting.py
Original file line number Diff line number Diff line change
Expand Up @@ -553,6 +553,7 @@ def _initialize_model_component(self, model_comp, comp_label, poly_order=None):
# equivs for spectral density and flux<>sb
pixar_sr = masked_spectrum.meta.get('_pixel_scale_factor', 1.0)
equivs = all_flux_unit_conversion_equivs(pixar_sr, init_x)
equivs += u.spectral_density(init_x)
Copy link
Member

Choose a reason for hiding this comment

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

should this be in all_flux_unit_conversion_equivs (or at least as an additional option)?


init_y = flux_conversion_general([init_y.value],
init_y.unit,
Expand Down Expand Up @@ -904,8 +905,17 @@ def _fit_model_to_spectrum(self, add_data):
return
models_to_fit = self._reinitialize_with_fixed()

masked_spectrum = self._apply_subset_masks(self.dataset.selected_spectrum,
spec = self.dataset.selected_spectrum
# Needs Attention:
# should conversion occur before or after call to _apply_subset_masks?
if spec.flux.unit.to_string != self.app._get_display_unit('flux'):
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this a method? Is it cheaper to compare units instead of casting to string?

Suggested change
if spec.flux.unit.to_string != self.app._get_display_unit('flux'):
if spec.flux.unit.to_string() != self.app._get_display_unit('flux'):

equivalencies = u.spectral_density(self.dataset.selected_spectrum.spectral_axis)
spec = self.dataset.selected_spectrum.with_flux_unit(self.app._get_display_unit('flux'),
equivalencies=equivalencies)
Comment on lines +909 to +914
Copy link
Member

Choose a reason for hiding this comment

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

should this all be moved into the DatasetSelect.selected_spectrum logic so its reusable and cached (and then the cache would need to be invalidated for a change in flux display units)?


masked_spectrum = self._apply_subset_masks(spec,
self.spectral_subset)

try:
fitted_model, fitted_spectrum = fit_model_to_spectrum(
masked_spectrum,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -311,6 +311,8 @@ def test_results_table(specviz_helper, spectrum1d):
mf = specviz_helper.plugins['Model Fitting']
mf.create_model_component('Linear1D')

uc = specviz_helper.plugins['Unit Conversion']

mf.add_results.label = 'linear model'
with warnings.catch_warnings():
warnings.filterwarnings('ignore', message='Model is linear in parameters.*')
Expand All @@ -324,6 +326,9 @@ def test_results_table(specviz_helper, spectrum1d):
'L:intercept_0', 'L:intercept_0:unit',
'L:intercept_0:fixed', 'L:intercept_0:std']

# verify units in table match the current display unit
assert mf_table['L:intercept_0:unit'][-1] == uc.flux_unit

mf.create_model_component('Gaussian1D')
mf.add_results.label = 'composite model'
with warnings.catch_warnings():
Expand All @@ -345,6 +350,19 @@ def test_results_table(specviz_helper, spectrum1d):
'G:stddev_1', 'G:stddev_1:unit',
'G:stddev_1:fixed', 'G:stddev_1:std']

mf.remove_model_component('G')
assert len(mf_table) == 2

# verify Spectrum1D model fitting plugin and table can handle spectral density conversions
uc.flux_unit = 'erg / (Angstrom s cm2)'
mf.reestimate_model_parameters()

with warnings.catch_warnings():
warnings.filterwarnings('ignore', message='Model is linear in parameters.*')
mf.calculate_fit(add_data=True)

assert mf_table['L:intercept_0:unit'][-1] == uc.flux_unit


def test_equation_validation(specviz_helper, spectrum1d):
data_label = 'test'
Expand Down
16 changes: 16 additions & 0 deletions jdaviz/configs/imviz/plugins/coords_info/coords_info.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
from jdaviz.core.unit_conversion_utils import (all_flux_unit_conversion_equivs,
check_if_unit_is_per_solid_angle,
flux_conversion_general)
from jdaviz.utils import flux_conversion

__all__ = ['CoordsInfo']

Expand Down Expand Up @@ -433,6 +434,12 @@ def _image_viewer_update(self, viewer, x, y):
# use WCS to expose the wavelength for a 2d spectrum shown in pixel space
try:
wave, pixel = image.coords.pixel_to_world(x, y)
if wave is not None:
equivalencies = all_flux_unit_conversion_equivs(cube_wave=wave)
wave = wave.to(self.app._get_display_unit('spectral'),
equivalencies=equivalencies)
self._dict['spectral_axis'] = wave.value
self._dict['spectral_axis:unit'] = wave.unit.to_string()
Comment on lines +441 to +442
Copy link
Member

Choose a reason for hiding this comment

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

where were these handled before (I don't see them in the else 🤔)?

except Exception: # WCS might not be valid # pragma: no cover
coords_status = False
else:
Expand Down Expand Up @@ -483,12 +490,21 @@ def _image_viewer_update(self, viewer, x, y):

if isinstance(viewer, (ImvizImageView, MosvizImageView, MosvizProfile2DView)):
value = image.get_data(attribute)[int(round(y)), int(round(x))]

if associated_dq_layers is not None:
associated_dq_layer = associated_dq_layers[0]
dq_attribute = associated_dq_layer.state.attribute
dq_data = associated_dq_layer.layer.get_data(dq_attribute)
dq_value = dq_data[int(round(y)), int(round(x))]

unit = u.Unit(image.get_component(attribute).units)
if (isinstance(viewer, MosvizProfile2DView) and unit != ''
and unit != self.app._get_display_unit(attribute)):
equivalencies = all_flux_unit_conversion_equivs(cube_wave=wave)
value = flux_conversion(value, unit, self.app._get_display_unit(attribute),
eqv=equivalencies)
unit = self.app._get_display_unit(attribute)

elif isinstance(viewer, (CubevizImageView, RampvizImageView)):
arr = image.get_component(attribute).data
unit = u.Unit(image.get_component(attribute).units)
Expand Down
5 changes: 4 additions & 1 deletion jdaviz/configs/mosviz/tests/test_data_loading.py
Original file line number Diff line number Diff line change
Expand Up @@ -222,8 +222,11 @@ def test_load_single_image_multi_spec(mosviz_helper, mos_image, spectrum1d, mos_

label_mouseover._viewer_mouse_event(spec2d_viewer,
{'event': 'mousemove', 'domain': {'x': 10, 'y': 100}})

# Note: spectra2d Wave loaded in meters, but we respect one spectral unit, so the meters in
# converted to Angstrom (the spectra1d spectral unit).
Comment on lines +226 to +227
Copy link
Member

Choose a reason for hiding this comment

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

Can you explain this a little more? Is it that a previous data entry was in angstroms or just that that is the default spectral display unit?

Suggested change
# Note: spectra2d Wave loaded in meters, but we respect one spectral unit, so the meters in
# converted to Angstrom (the spectra1d spectral unit).
# Note: spectra2d Wave loaded in meters, but we respect one spectral unit, so the meters is
# converted to Angstrom (the spectra1d spectral unit).

assert label_mouseover.as_text() == ('Pixel x=00010.0 y=00100.0 Value +8.12986e-01',
'Wave 1.10000e-05 m', '')
'Wave 1.10000e+05 Angstrom', '')
assert label_mouseover.icon == 'c'

# need to trigger a mouseleave or mouseover to reset the traitlets
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from numpy.testing import assert_allclose
from regions import RectanglePixelRegion, PixCoord
from specutils import Spectrum1D, SpectralRegion
from glue.core.roi import XRangeROI

from jdaviz.configs.specviz.plugins.line_analysis.line_analysis import _coerce_unit
from jdaviz.core.custom_units_and_equivs import PIX2
Expand Down Expand Up @@ -91,7 +92,7 @@ def test_cubeviz_units(cubeviz_helper, spectrum1d_cube_custom_fluxunit,
is in flux/pix2 and flux/sr, and that the results remain consistant
between translations of the spectral y axis flux<>surface brightness.
"""
cube = spectrum1d_cube_custom_fluxunit(fluxunit=u.Jy / sq_angle_unit,
cube = spectrum1d_cube_custom_fluxunit(fluxunit=u.MJy / sq_angle_unit,
shape=(25, 25, 4), with_uncerts=True)
cubeviz_helper.load_data(cube, data_label="Test Cube")

Expand All @@ -107,9 +108,46 @@ def test_cubeviz_units(cubeviz_helper, spectrum1d_cube_custom_fluxunit,
results = plugin.results
assert results[0]['unit'] == 'W / m2'

viewer = cubeviz_helper.app.get_viewer('spectrum-viewer')
viewer.apply_roi(XRangeROI(4.63e-7, 4.64e-7))

la = cubeviz_helper.plugins['Line Analysis']
la.keep_active = True
la.spectral_subset.selected = 'Subset 1'

marks_before = [la._obj.continuum_marks['left'].y,
la._obj.continuum_marks['right'].y]

# change flux unit and make sure result stays the same after conversion
uc.flux_unit.selected = 'Jy'

marks_after = [la._obj.continuum_marks['left'].y,
la._obj.continuum_marks['right'].y]

# ensure continuum marks update when spectral_y is changed by
# multiply converted continuum marks by expected scale factor (MJy -> Jy)
scaling_factor = 1e6
assert_allclose([mark * scaling_factor for mark in marks_before], marks_after, rtol=1e-5)

# reset to test again after spectral_y_type is changed
marks_before = marks_after

# now change to surface brightness
uc.spectral_y_type = 'Surface Brightness'

if sq_angle_unit == PIX2:
# translation does not alter spectral_y values in viewer
scaling_factor = 1
else:
scaling_factor = cube.meta.get('PIXAR_SR')

marks_after = [la._obj.continuum_marks['left'].y,
la._obj.continuum_marks['right'].y]

# ensure continuum marks update when spectral_y_type is changed
# multiply converted continuum marks by expected pixel scale factor
assert_allclose([mark / scaling_factor for mark in marks_before], marks_after, rtol=1e-5)

results = plugin.results
line_flux_before_unit_conversion = results[0]
# convert back and forth between unit<>str for string format consistency
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -154,3 +154,40 @@ def test_flux_unit_choices(specviz_helper, flux_unit, expected_choices):

assert uc_plg.flux_unit.selected == flux_unit.to_string()
assert uc_plg.flux_unit.choices == expected_choices


def test_mosviz_profile_view_mouseover(specviz2d_helper, spectrum2d):
data = np.zeros((5, 10))
data[3] = np.arange(10)
spectrum2d = Spectrum1D(flux=data*u.MJy, spectral_axis=data[3]*u.um)

specviz2d_helper.load_data(spectrum2d)
viewer = specviz2d_helper.app.get_viewer("spectrum-viewer")
plg = specviz2d_helper.plugins["Unit Conversion"]

# make sure we don't expose angle, sb, nor spectral-y units when native
# units are in flux
assert hasattr(plg, 'flux_unit')
assert not hasattr(plg, 'angle_unit')
assert not hasattr(plg, 'sb_unit')
assert not hasattr(plg, 'spectral_y_type')

label_mouseover = specviz2d_helper.app.session.application._tools['g-coords-info']
label_mouseover._viewer_mouse_event(viewer,
{'event': 'mousemove',
'domain': {'x': 5, 'y': 3}})

assert label_mouseover.as_text() == ('Cursor 5.00000e+00, 3.00000e+00',
'Wave 5.00000e+00 um (5 pix)',
'Flux 5.00000e+00 MJy')

plg._obj.flux_unit_selected = 'Jy'
assert label_mouseover.as_text() == ('Cursor 5.00000e+00, 3.00000e+00',
'Wave 5.00000e+00 um (5 pix)',
'Flux 5.00000e+06 Jy')

# test mouseover when spectral density equivalencies are required for conversion
plg._obj.flux_unit_selected = 'erg / (Angstrom s cm2)'
assert label_mouseover.as_text() == ('Cursor 5.00000e+00, 3.00000e+00',
'Wave 5.00000e+00 um (5 pix)',
'Flux 5.99585e-08 erg / (Angstrom s cm2)')
Loading
Loading