-
Notifications
You must be signed in to change notification settings - Fork 75
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
base: main
Are you sure you want to change the base?
Conversation
Is a regression test easy to add? Both you and Ricky seemed surprised that CI didn't catch this. |
jdaviz/configs/specviz/plugins/unit_conversion/unit_conversion.py
Outdated
Show resolved
Hide resolved
jdaviz/configs/specviz/plugins/unit_conversion/unit_conversion.py
Outdated
Show resolved
Hide resolved
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3144 +/- ##
==========================================
- Coverage 88.82% 88.80% -0.02%
==========================================
Files 112 112
Lines 17550 17552 +2
==========================================
- Hits 15588 15587 -1
- Misses 1962 1965 +3 ☔ View full report in Codecov by Sentry. |
6ac80f6
to
63f895d
Compare
jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py
Show resolved
Hide resolved
axis = {'spectral': 'x', 'flux': 'y'}.get(msg.axis) | ||
axis = {'spectral': 'x', 'spectral_y': 'y'}.get(msg.axis) |
There was a problem hiding this comment.
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
.
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 |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
jdaviz/configs/cubeviz/plugins/spectral_extraction/spectral_extraction.py
Show resolved
Hide resolved
@@ -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) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
handler=self._on_gloabl_display_unit_changed) | |
handler=self._on_global_display_unit_changed) |
@@ -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={}): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
def _on_gloabl_display_unit_changed(self, msg={}): | |
def _on_global_display_unit_changed(self, msg={}): |
This PR improves the messaging from the unit conversion plugin to broadcast changes in selected display unit. Because some plugins are listening for changes to the spectral y axis (either from toggling flux<>sb, changing flux or angle when it is toggled to SB, or changing flux when it is toggled to flux) this is its own message now.
Every time a new selection of 'flux' is made, there are three GlobalDisplayUnitChanged messages broadcasted : 'flux', 'sb', and 'spectral_y'.
While this is not included in this PR, these changes will make it possible for the correct messaging to be broadcasted when a new angle selection is made (always sending a 'sb' message, and if the spectral y axis is in SB also sending a 'spectral_y' message).
Every time there is a toggle between flux<> sb, the GlobalDisplayUnitChanged axis='spectral_y' message is broadcasted.
This PR also updates the plugins listening to unit changes to intercept only the relevant type of GlobalDisplayUnitChanged - before, they were responding to any message of this type.
Finally, this fixes an issue with mouseover in cubeviz and adds test coverage.