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

update unit conversion message responses in plugins, add tests #3144

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 6 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,8 @@ class SpectralExtraction(PluginTemplateMixin, ApertureSubsetSelectMixin,

results_units = Unicode().tag(sync=True)
spectrum_y_units = Unicode().tag(sync=True)
flux_unit = Unicode().tag(sync=True)
sb_unit = Unicode().tag(sync=True)

aperture_method_items = List().tag(sync=True)
aperture_method_selected = Unicode('Center').tag(sync=True)
Expand Down Expand Up @@ -178,7 +180,7 @@ def __init__(self, *args, **kwargs):
self.session.hub.subscribe(self, SliceValueUpdatedMessage,
handler=self._on_slice_changed)
self.hub.subscribe(self, GlobalDisplayUnitChanged,
handler=self._update_results_units)
handler=self._on_gloabl_display_unit_changed)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
handler=self._on_gloabl_display_unit_changed)
handler=self._on_global_display_unit_changed)


self._update_disabled_msg()

Expand Down Expand Up @@ -313,13 +315,27 @@ def _update_mark_scale(self, *args):
else:
self.background.scale_factor = self.slice_spectral_value/self.reference_spectral_value

def _on_gloabl_display_unit_changed(self, msg={}):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
def _on_gloabl_display_unit_changed(self, msg={}):
def _on_global_display_unit_changed(self, msg={}):


if msg.axis == 'spectral_y':
self.spectrum_y_units = str(msg.unit)

# a 'flux' and 'sb' message should be recieved back to back from
# the unit conversion plugin, so don't need to sync them immediatley
# within each message recieved
if msg.axis == 'flux':
self.flux_unit = str(msg.unit)
if msg.axis == 'sb':
self.sb_unit = str(msg.unit)

@observe('function_selected')
def _update_results_units(self, msg={}):
self.spectrum_y_units = str(self.app._get_display_unit('spectral_y'))
def _update_units_on_function_selection(self, *args):

if self.function_selected.lower() == 'sum':
self.results_units = str(self.app._get_display_unit('flux'))
self.results_units = self.flux_unit
else:
self.results_units = str(self.app._get_display_unit('sb'))
self.results_units = self.sb_unit
kecnry marked this conversation as resolved.
Show resolved Hide resolved


@observe('function_selected', 'aperture_method_selected')
def _update_aperture_method_on_function_change(self, *args):
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -528,7 +528,7 @@ def _check_model_component_compat(self, axes=['x', 'y'], display_units=None):
self._check_model_equation_invalid()

def _on_global_display_unit_changed(self, msg):
axis = {'spectral': 'x', 'flux': 'y'}.get(msg.axis)
axis = {'spectral': 'x', 'spectral_y': 'y'}.get(msg.axis)
Copy link
Member

Choose a reason for hiding this comment

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

this probably needs some logic to skip if the msg.axis is not either spectral or spectral_y (could be by checking if axis is None after the get, or rewriting this into if, elif, else: return.


# update internal tracking of current units
self._units[axis] = str(msg.unit)
Expand Down
18 changes: 14 additions & 4 deletions jdaviz/configs/imviz/plugins/coords_info/coords_info.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,13 @@
from jdaviz.configs.mosviz.plugins.viewers import (MosvizImageView, MosvizProfileView,
MosvizProfile2DView)
from jdaviz.configs.specviz.plugins.viewers import SpecvizProfileView
from jdaviz.core.events import ViewerAddedMessage, GlobalDisplayUnitChanged
from jdaviz.core.events import ViewerAddedMessage, GlobalDisplayUnitChanged, SnackbarMessage
from jdaviz.core.helpers import data_has_valid_wcs
from jdaviz.core.marks import PluginScatter, PluginLine
from jdaviz.core.registries import tool_registry
from jdaviz.core.template_mixin import TemplateMixin, DatasetSelectMixin
from jdaviz.utils import _eqv_pixar_sr, _convert_surface_brightness_units
from jdaviz.core.validunits import check_if_unit_is_per_solid_angle

__all__ = ['CoordsInfo']

Expand Down Expand Up @@ -120,9 +121,18 @@ def _on_viewer_added(self, msg):
self._create_viewer_callbacks(self.app.get_viewer_by_id(msg.viewer_id))

def _on_global_display_unit_changed(self, msg):
# eventually should observe change in flux OR angle
if msg.axis == "flux":
self.image_unit = u.Unit(msg.unit)

# should only listen to changes in surface brightness. even if data
# loaded is in 'flux' it can be represented as a per-pixel surface
# brightness unit.
if msg.axis == "sb":
image_unit = u.Unit(msg.unit)

# temporarily, until non-sr units are suppported, strip 'pix' from unit
if 'pix' in image_unit.bases:
image_unit = image_unit * u.pix

self.image_unit = u.Unit(image_unit)

@property
def marks(self):
Expand Down
33 changes: 20 additions & 13 deletions jdaviz/configs/specviz/plugins/unit_conversion/unit_conversion.py
Original file line number Diff line number Diff line change
Expand Up @@ -230,21 +230,22 @@ def _on_flux_unit_changed(self, msg):
if not self.flux_unit.choices and self.app.config == 'cubeviz':
return

flux_or_sb = None

# when the configuration is Specviz, translation is not currently supported.
# If in Cubeviz, all spectra pass through Spectral Extraction plugin and will
# have a scale factor assigned in the metadata, enabling translation.
current_y_unit = self.spectrum_viewer.state.y_display_unit

# if the current y display unit is a surface brightness unit,
if self.angle_unit.selected and check_if_unit_is_per_solid_angle(current_y_unit):
flux_or_sb = self._append_angle_correctly(
self.flux_unit.selected,
self.angle_unit.selected
)
else:
flux_or_sb = self.flux_unit.selected
# various plugins are listening for changes in either flux or sb and
# need to be able to filter messages accordingly, so broadcast both when
# flux unit is updated. if data was loaded in a flux unit (i.e MJy), it
# can be reperesented as a per-pixel surface brightness unit
flux_unit = self.flux_unit.selected
sb_unit = self._append_angle_correctly(flux_unit, self.angle_unit.selected)

self.hub.broadcast(GlobalDisplayUnitChanged("flux", flux_unit, sender=self))
self.hub.broadcast(GlobalDisplayUnitChanged("sb", sb_unit, sender=self))

flux_or_sb = sb_unit if check_if_unit_is_per_solid_angle(current_y_unit) else flux_unit
Copy link
Member

Choose a reason for hiding this comment

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

Is there any reason this can't use the self.flux_or_sb switch instead of self.spectrum_viewer.state.y_display_unit (maybe @gibsongreen would have some context on that as well)? That could be done as a follow-up if its out of scope.

Either way, maybe we rename the variable to spectral_y to be consistent with the message that is broadcast from this and to avoid confusion with the switch itself?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that should work, the traitlet for self.flux_or_sb at this point in the function should still match that flux or surface brightness selection for the value of self.spectrum_viewer.state.y_display_unit/spectral_y/current_y_unit


untranslatable_units = self._untranslatable_units
# disable translator if flux unit is untranslatable,
Expand All @@ -262,8 +263,10 @@ def _on_flux_unit_changed(self, msg):
self.spectrum_viewer.state.y_display_unit = yunit
self.spectrum_viewer.reset_limits()

# and broacast that there has been a change in flux
self.hub.broadcast(GlobalDisplayUnitChanged("flux", flux_or_sb, sender=self))
# and broacast that there has been a change in the spectral axis y unit
# to either a flux or surface brightness unit, for plugins that specifically
# care about this toggle selection
self.hub.broadcast(GlobalDisplayUnitChanged("spectral_y", flux_or_sb, sender=self))

if not check_if_unit_is_per_solid_angle(self.spectrum_viewer.state.y_display_unit):
self.flux_or_sb_selected = 'Flux'
Expand Down Expand Up @@ -298,10 +301,12 @@ def _translate(self, flux_or_sb=None):
spec_units = u.Unit(self.spectrum_viewer.state.y_display_unit)
else:
return

# on instantiation, we set determine flux choices and selection
# after surface brightness
if not self.flux_unit.choices:
return

# Surface Brightness -> Flux
if check_if_unit_is_per_solid_angle(spec_units) and flux_or_sb == 'Flux':
spec_units *= u.sr
Expand All @@ -318,7 +323,9 @@ def _translate(self, flux_or_sb=None):
else:
return

self.hub.broadcast(GlobalDisplayUnitChanged('flux',
# broadcast that there has been a change in the spectrum viewer y axis,
# if translation was completed
self.hub.broadcast(GlobalDisplayUnitChanged('spectral_y',
spec_units,
sender=self))
self.spectrum_viewer.reset_limits()
Expand Down
5 changes: 4 additions & 1 deletion jdaviz/core/events.py
Original file line number Diff line number Diff line change
Expand Up @@ -408,7 +408,10 @@ def flip_horizontal(self):


class GlobalDisplayUnitChanged(Message):
'''Message generated when the global app-wide display unit is changed'''
'''Message generated when the (x or y axis) unit of the spectrum viewer is
changed, which is used app-wide to inform display units that depend on the
unit choice and flux<>sb toggle of the spectrum viewer.'''

def __init__(self, axis, unit, *args, **kwargs):
super().__init__(*args, **kwargs)
self._axis = axis
Expand Down
2 changes: 1 addition & 1 deletion jdaviz/core/marks.py
Original file line number Diff line number Diff line change
Expand Up @@ -127,7 +127,7 @@ def _on_global_display_unit_changed(self, msg):
if not self.auto_update_units:
return
if self.viewer.__class__.__name__ in ['SpecvizProfileView', 'CubevizProfileView']:
axis_map = {'spectral': 'x', 'flux': 'y'}
axis_map = {'spectral': 'x', 'spectral_y': 'y'}
elif self.viewer.__class__.__name__ == 'MosvizProfile2DView':
axis_map = {'spectral': 'x'}
else:
Expand Down
2 changes: 1 addition & 1 deletion jdaviz/core/template_mixin.py
Original file line number Diff line number Diff line change
Expand Up @@ -3547,7 +3547,7 @@ def _dc_to_dict(data):
self._clear_cache(*self._cached_properties)

def _on_global_display_unit_changed(self, msg=None):
if msg.axis in ('spectral', 'flux'):
if msg.axis in ('spectral', 'spectral_y'):
self._clear_cache('selected_spectrum')


Expand Down
Loading