Update the mouse over units by listening to the unit-conversion plugin #2931

haticekaratay · 2024-06-28T17:27:07Z

Description

This pull request adds the functionality for the mouse over on the image viewers to listen to the unit conversion plugin.

Fixes #

Change log entry

Is a change log needed? If yes, is it added to CHANGES.rst? If you want to avoid merge conflicts,
list the proposed change log here for review and add to CHANGES.rst before merge. If no, maintainer
should add a no-changelog-entry-needed label.

Checklist for package maintainer(s)

This checklist is meant to remind the package maintainer(s) who will review this pull request of some common things to look for. This list is not exhaustive.

Are two approvals required? Branch protection rule does not check for the second approval. If a second approval is not necessary, please apply the trivial label.
Do the proposed changes actually accomplish desired goals? Also manually run the affected example notebooks, if necessary.
Do the proposed changes follow the STScI Style Guides?
Are tests added/updated as required? If so, do they follow the STScI Style Guides?
Are docs added/updated as required? If so, do they follow the STScI Style Guides?
Did the CI pass? If not, are the failures related?
Is a milestone set? Set this to bugfix milestone if this is a bug fix and needs to be released ASAP; otherwise, set this to the next major release milestone. Bugfix milestone also needs an accompanying backport label.
After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

jdaviz/configs/imviz/plugins/coords_info/coords_info.py

rosteen · 2024-07-26T17:12:42Z

It looks to me like the remaining test failures are because current_unit is getting initialized to MJy/sr, so if no unit conversion is done that's the unit getting shown in the label no matter what the unit of the data is. So you're getting things like moments in km/s getting shown in the coord info display with the correct value, but MJy/sr for the unit.

codecov · 2024-07-27T03:18:13Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.91%. Comparing base (143cd8d) to head (14928f6).
Report is 3 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2931      +/-   ##
==========================================
+ Coverage   88.78%   88.91%   +0.12%     
==========================================
  Files         111      111              
  Lines       17372    17386      +14     
==========================================
+ Hits        15424    15459      +35     
+ Misses       1948     1927      -21

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

jdaviz/utils.py

gibsongreen · 2024-07-29T14:12:02Z

The code is clean and the tests make sure the data is being converted from sb to sb. The only thing that I’m seeing is what Ricky pointed out.

There is one thing that I hadn’t considered and so definitely not in the scope of your ticket. When we create subsets, depending on what viewer it I would think we would want the unit of that subset to be based on what viewer it was created in. Right now I think it is listening to that data collection (which doesn’t update from conversions or translation). That could get messy if the user creates subsets with a bunch of different units, but I'll look into this.

jdaviz/utils.py

jdaviz/configs/specviz/plugins/unit_conversion/tests/test_unit_conversion.py

jdaviz/utils.py

jdaviz/configs/imviz/plugins/coords_info/coords_info.py

jdaviz/utils.py

jdaviz/configs/imviz/plugins/coords_info/coords_info.py

kecnry · 2024-07-30T14:02:07Z

jdaviz/configs/imviz/plugins/coords_info/coords_info.py

+                value = self._get_cube_value(
+                    image, arr, x, y, viewer
+                )
+                if self.current_unit is not None and self.current_unit.is_equivalent(unit):


this seems to be the logic that keeps velocity mouseover working for moment maps. Just a note that we'll need to rethink this if/when we have velocity display units as well, since we'll then need to have per-viewer image units based on each viewers "physical type" and its corresponding display units 🤔

Co-authored-by: P. L. Lim <[email protected]>

kecnry

Looks great - thanks!

kecnry · 2024-07-30T18:41:45Z

jdaviz/configs/specviz/plugins/unit_conversion/unit_conversion.py

-            self.hub.broadcast(GlobalDisplayUnitChanged('flux',
-                                                        flux_or_sb,
-                                                        sender=self))
+            self.hub.broadcast(
+                GlobalDisplayUnitChanged(
+                    "flux" if name == "flux_unit_selected" else "sb", flux_or_sb, sender=self
+                    )
+                )


@gibsongreen - does this affect you in #3111? Will we need to emit messages for (flux AND surface brightness) OR (solid angle AND surface brightness)?

I believe it will be solid_angle and flux, but in my PR solid_angle isn't computed/changed in this function currently (since it is either /sr if surface brightness, or, / pix is the cube loaded is flux (which really should spaxel). Right now it is computed here:

jdaviz/jdaviz/configs/specviz/plugins/unit_conversion/unit_conversion.py

Line 181 in 196659b

self.angle_unit.choices = create_angle_equivalencies_list(dc_unit)

When enabling changes to the solid_angle unit then there would be addition of something like solid_angle_unit_selected that would need to be observed in this function

rosteen

Looks good to me now, thanks!

jdaviz/configs/imviz/plugins/coords_info/coords_info.py

github-actions bot added imviz plugin Label for plugins common to multiple configurations labels Jun 28, 2024

haticekaratay added this to the 4.0 milestone Jul 12, 2024

haticekaratay added 3 commits July 23, 2024 15:56

Respond to changed unit and display on the UI

f936342

Convert all sb<>sb units listed in the dropdown menu

3851292

Use the default current unit from the cube

dae87e7

haticekaratay force-pushed the mouse-over-units branch from a70ec65 to dae87e7 Compare July 23, 2024 19:58

kecnry reviewed Jul 24, 2024

View reviewed changes

jdaviz/configs/imviz/plugins/coords_info/coords_info.py Outdated Show resolved Hide resolved

jdaviz/configs/imviz/plugins/coords_info/coords_info.py Outdated Show resolved Hide resolved

kecnry reviewed Jul 24, 2024

View reviewed changes

jdaviz/configs/imviz/plugins/coords_info/coords_info.py Outdated Show resolved Hide resolved

Apply only for relevant configs

969bd3d

haticekaratay changed the title ~~WIP: Respond to changed unit and display on the UI~~ Update the mouse over units by listening to the unit-conversion plugin Jul 25, 2024

haticekaratay added 2 commits July 26, 2024 14:08

Update current_unit setting logic

c550e42

Fix failing tests

d8b5e44

haticekaratay marked this pull request as ready for review July 27, 2024 03:12

haticekaratay requested review from rosteen, javerbukh, pllim, bmorris3 and cshanahan1 as code owners July 27, 2024 03:12

haticekaratay added 4 commits July 28, 2024 23:11

Add tests

fd9f7f4

Add a changelog

6471cbc

Move conversion logic to utils

77f9069

Move the conversion to utils

81a723e

rosteen reviewed Jul 29, 2024

View reviewed changes

jdaviz/utils.py Outdated Show resolved Hide resolved

Pass when unable to convert

8a086a6

pllim reviewed Jul 29, 2024

View reviewed changes

jdaviz/utils.py Outdated Show resolved Hide resolved

pllim reviewed Jul 29, 2024

View reviewed changes

jdaviz/configs/specviz/plugins/unit_conversion/tests/test_unit_conversion.py Outdated Show resolved Hide resolved

pllim reviewed Jul 29, 2024

View reviewed changes

jdaviz/utils.py Outdated Show resolved Hide resolved

pllim reviewed Jul 29, 2024

View reviewed changes

jdaviz/configs/imviz/plugins/coords_info/coords_info.py Outdated Show resolved Hide resolved

pllim reviewed Jul 29, 2024

View reviewed changes

jdaviz/configs/imviz/plugins/coords_info/coords_info.py Outdated Show resolved Hide resolved

haticekaratay added 4 commits July 29, 2024 15:18

Update tests

25e3543

Update based on the feedback

9d64af7

Only subscribe to cubeviz

14928f6

Check for equivalency rather than image dimension

c48f7e2

pllim reviewed Jul 30, 2024

View reviewed changes

jdaviz/utils.py Outdated Show resolved Hide resolved

jdaviz/utils.py Show resolved Hide resolved

kecnry reviewed Jul 30, 2024

View reviewed changes

jdaviz/configs/imviz/plugins/coords_info/coords_info.py Outdated Show resolved Hide resolved

kecnry reviewed Jul 30, 2024

View reviewed changes

jdaviz/configs/imviz/plugins/coords_info/coords_info.py Outdated Show resolved Hide resolved

kecnry reviewed Jul 30, 2024

View reviewed changes

haticekaratay and others added 3 commits July 30, 2024 13:55

Refactor conversion logic

e4d6f86

Co-authored-by: P. L. Lim <[email protected]>

Rename unit and broadcast 'sb' as part of msg

8b3c808

Code style

7903290

kecnry approved these changes Jul 30, 2024

View reviewed changes

kecnry added the Ready for final review label Jul 30, 2024

kecnry reviewed Jul 30, 2024

View reviewed changes

rosteen approved these changes Jul 30, 2024

View reviewed changes

jdaviz/configs/imviz/plugins/coords_info/coords_info.py Outdated Show resolved Hide resolved

haticekaratay merged commit a126296 into spacetelescope:main Jul 30, 2024
25 checks passed

gibsongreen mentioned this pull request Jul 30, 2024

Surface brightness -> solid angle conversion updates #3111

Merged

13 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update the mouse over units by listening to the unit-conversion plugin #2931

Update the mouse over units by listening to the unit-conversion plugin #2931

haticekaratay commented Jun 28, 2024 •

edited

Loading

rosteen commented Jul 26, 2024

codecov bot commented Jul 27, 2024 •

edited

Loading

gibsongreen commented Jul 29, 2024 •

edited

Loading

kecnry Jul 30, 2024

kecnry left a comment

kecnry Jul 30, 2024

gibsongreen Jul 30, 2024 •

edited

Loading

rosteen left a comment

Update the mouse over units by listening to the unit-conversion plugin #2931

Update the mouse over units by listening to the unit-conversion plugin #2931

Conversation

haticekaratay commented Jun 28, 2024 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

rosteen commented Jul 26, 2024

codecov bot commented Jul 27, 2024 • edited Loading

Codecov Report

gibsongreen commented Jul 29, 2024 • edited Loading

kecnry Jul 30, 2024

Choose a reason for hiding this comment

kecnry left a comment

Choose a reason for hiding this comment

kecnry Jul 30, 2024

Choose a reason for hiding this comment

gibsongreen Jul 30, 2024 • edited Loading

Choose a reason for hiding this comment

rosteen left a comment

Choose a reason for hiding this comment

haticekaratay commented Jun 28, 2024 •

edited

Loading

codecov bot commented Jul 27, 2024 •

edited

Loading

gibsongreen commented Jul 29, 2024 •

edited

Loading

gibsongreen Jul 30, 2024 •

edited

Loading