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

display units: fix failing tests #2132

Merged
merged 12 commits into from
Apr 13, 2023
4 changes: 2 additions & 2 deletions jdaviz/app.py
Original file line number Diff line number Diff line change
Expand Up @@ -886,8 +886,8 @@ def get_subsets(self, subset_name=None, spectral_only=False,
object_only : bool
Return only object relevant information and
leave out the region class name and glue_state.
use_display_units: bool, optional
Whether to convert to the display units defined in the <unit-conversion> plugin.
use_display_units : bool, optional
Whether to convert to the display units defined in the Unit Conversion plugin.

Returns
-------
Expand Down
3 changes: 2 additions & 1 deletion jdaviz/configs/cubeviz/plugins/slice/slice.py
Original file line number Diff line number Diff line change
Expand Up @@ -171,7 +171,8 @@ def _on_global_display_unit_changed(self, msg):
return
prev_unit = self.wavelength_unit
self.wavelength_unit = msg.unit.to_string()
if self._x_all is None or prev_unit == '':
# original unit during init can be blank or deg (before axis is set correctly)
if self._x_all is None or prev_unit in ('deg', ''):
return
self._update_data((self._x_all * u.Unit(prev_unit)).to(msg.unit, u.spectral()))

Expand Down
1 change: 1 addition & 0 deletions jdaviz/configs/mosviz/mosviz.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ tray:
- g-gaussian-smooth
- g-slit-overlay
- g-model-fitting
- g-unit-conversion
- g-line-list
- specviz-line-analysis
- g-export-plot
Expand Down
9 changes: 7 additions & 2 deletions jdaviz/configs/specviz/helper.py
Original file line number Diff line number Diff line change
Expand Up @@ -128,18 +128,23 @@ def get_spectra(self, data_label=None, subset_to_apply=None, apply_slider_redshi

return output_spectra

def get_spectral_regions(self):
def get_spectral_regions(self, use_display_units=False):
Copy link
Member Author

Choose a reason for hiding this comment

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

we might want to discuss the default option here and in get_subsets (but can change that anytime before merging the dev unit conversion branch into main)

Copy link
Contributor

Choose a reason for hiding this comment

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

I can see arguments both ways but being a data purist, I would want the regions to be in data unit, not display unit.

"""
A simple wrapper around the app-level call to retrieve only spectral
subsets, which are now returned as SpectralRegions by default.

Parameters
----------
use_display_units : bool, optional
Whether to convert to the display units defined in the Unit Conversion plugin.

Returns
-------
spec_regs : dict
Mapping from the names of the subsets to the subsets expressed
as `specutils.SpectralRegion` objects.
"""
return self.app.get_subsets(spectral_only=True)
return self.app.get_subsets(spectral_only=True, use_display_units=use_display_units)

def x_limits(self, x_min=None, x_max=None):
"""Sets the limits of the x-axis
Expand Down
9 changes: 6 additions & 3 deletions jdaviz/configs/specviz/plugins/viewers.py
Original file line number Diff line number Diff line change
Expand Up @@ -361,8 +361,10 @@ def add_data(self, data, color=None, alpha=None, **layer_state):
result = super().add_data(data, color, alpha, **layer_state)

if reset_plot_axes:
self.state.x_display_unit = data.get_component(self.state.x_att.label).units
self.state.y_display_unit = data.get_component("flux").units
x_units = data.get_component(self.state.x_att.label).units
y_units = data.get_component("flux").units
self.state.x_display_unit = x_units if len(x_units) else None
self.state.y_display_unit = y_units if len(y_units) else None
Comment on lines +366 to +367
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think len is needed.

Suggested change
self.state.x_display_unit = x_units if len(x_units) else None
self.state.y_display_unit = y_units if len(y_units) else None
self.state.x_display_unit = x_units if x_units else None
self.state.y_display_unit = y_units if y_units else None

Copy link
Member Author

Choose a reason for hiding this comment

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

woops, turns out it was necessary (CI fails everywhere otherwise)

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, weird, but ok.

self.set_plot_axes()

self._plot_uncertainties()
Expand Down Expand Up @@ -489,7 +491,8 @@ def _plot_uncertainties(self):
def set_plot_axes(self):
# Set axes labels for the spectrum viewer
flux_unit_type = "Flux density"
x_unit = u.Unit(self.state.x_display_unit)
x_disp_unit = self.state.x_display_unit
x_unit = u.Unit(x_disp_unit) if x_disp_unit else u.dimensionless_unscaled
if x_unit.is_equivalent(u.m):
spectral_axis_unit_type = "Wavelength"
elif x_unit.is_equivalent(u.Hz):
Expand Down
16 changes: 10 additions & 6 deletions jdaviz/configs/specviz/tests/test_helper.py
Original file line number Diff line number Diff line change
Expand Up @@ -293,17 +293,21 @@ def test_get_spectral_regions_unit_conversion(specviz_helper, spectrum1d):
assert label_mouseover.icon == ''

# Convert the wavelength axis to micron
new_spectral_axis = "micron"
spec_viewer.state.x_display_unit = new_spectral_axis
spec_viewer.set_plot_axes()
new_spectral_axis = "um"
specviz_helper.plugins['Unit Conversion'].spectral_unit = new_spectral_axis

spec_viewer.apply_roi(XRangeROI(0.6, 0.7))

# Retrieve the Subset
subsets = specviz_helper.get_spectral_regions()
subsets = specviz_helper.get_spectral_regions(use_display_units=False)
reg = subsets.get('Subset 1')
assert reg.lower.unit == u.Angstrom
assert reg.upper.unit == u.Angstrom

subsets = specviz_helper.get_spectral_regions(use_display_units=True)
reg = subsets.get('Subset 1')
assert reg.lower.unit == u.micron
assert reg.upper.unit == u.micron
assert reg.lower.unit == u.um
assert reg.upper.unit == u.um

# Coordinates info panel should show new unit
label_mouseover._viewer_mouse_event(spec_viewer,
Expand Down
9 changes: 8 additions & 1 deletion jdaviz/core/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -415,13 +415,20 @@ def _handle_display_units(data, use_display_units):
spectral_unit = self.app._get_display_unit('spectral')
if not spectral_unit:
return data
if self.app.config == 'cubeviz' and spectral_unit == 'deg':
# this happens before the correct axis is set for the spectrum-viewer
# and would result in a unit-conversion error if attempting to convert
# to the display units. This should only ever be temporary during
# app intialization.
return data
flux_unit = self.app._get_display_unit('flux')
# TODO: any other attributes (meta, wcs, etc)?
# TODO: implement uncertainty.to upstream
new_uncert = data.uncertainty.__class__(data.uncertainty.quantity.to(flux_unit)) if data.uncertainty is not None else None # noqa
Copy link
Contributor

Choose a reason for hiding this comment

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

Wut... the uncertainty object cannot convert unit natively?

Copy link
Member Author

Choose a reason for hiding this comment

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

nope! I put a todo comment above, but probably should file an issue upstream. Probably in astropy.nddata?

data = Spectrum1D(spectral_axis=data.spectral_axis.to(spectral_unit,
u.spectral()),
flux=data.flux.to(flux_unit),
uncertainty=data.uncertainty.__class__(data.uncertainty.quantity.to(flux_unit))) # noqa
uncertainty=new_uncert)
else: # pragma: nocover
raise NotImplementedError(f"converting {data.__class__.__name__} to display units is not supported") # noqa
return data
Expand Down
2 changes: 1 addition & 1 deletion jdaviz/core/marks.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@

__all__ = ['OffscreenLinesMarks', 'BaseSpectrumVerticalLine', 'SpectralLine',
'SliceIndicatorMarks', 'ShadowMixin', 'ShadowLine', 'ShadowLabelFixedY',
'PluginMark', 'PluginLine', 'PluginScatter',
'PluginMark', 'LinesAutoUnit', 'PluginLine', 'PluginScatter',
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not look related.

Copy link
Member Author

Choose a reason for hiding this comment

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

its needed for RTD to be able to pull in the API docs for the classes that inherit from it.

'LineAnalysisContinuum', 'LineAnalysisContinuumCenter',
'LineAnalysisContinuumLeft', 'LineAnalysisContinuumRight',
'LineUncertainties', 'ScatterMask', 'SelectedSpaxel', 'MarkersMark']
Expand Down
1 change: 1 addition & 0 deletions jdaviz/core/template_mixin.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@


__all__ = ['show_widget', 'TemplateMixin', 'PluginTemplateMixin',
'ViewerPropertiesMixin',
Copy link
Contributor

Choose a reason for hiding this comment

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

This does not look related.

Copy link
Member Author

Choose a reason for hiding this comment

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

(same as above - needed for RTD)

'BasePluginComponent', 'SelectPluginComponent', 'UnitSelectPluginComponent',
'PluginSubcomponent',
'SubsetSelect', 'SpatialSubsetSelectMixin', 'SpectralSubsetSelectMixin',
Expand Down
4 changes: 2 additions & 2 deletions setup.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,8 @@ install_requires =
traitlets>=5.0.5
bqplot>=0.12.37
bqplot-image-gl>=1.4.11
glue-core>=1.9.0
glue-jupyter>=0.15.0
glue-core>=1.9.1
glue-jupyter>=0.16.2
echo>=0.5.0
ipykernel>=6.19.4
ipyvue>=1.6
Expand Down