Untranslatable unit handling #3139

gibsongreen · 2024-08-07T20:58:46Z

Description

This pull request is to address implementing a solution to handle units that cannot be translated using the MJy <-> MJy / sr custom equivalency (and subsequent surface brightness conversions between these units). The following units are able to be converted between flux units and other flux units, but issues specifically arise in translations and in surface brightness to surface brightness conversions. Custom equivalencies have been added in this PR that enable conversions and translations to solve this issue, and they work in a notebook cell setting. However, attempting to utilize this solution in Cubeviz UI causes tracebacks. It is likely due to the native units from the data collection. The native units will be used for the Spectrum1D object used in flux_conversion, and the native unit will also be used as either the original_unit or the target_unit (see jdaviz/utils.py). Translation(s) and/or conversion(s) needs to take place before the next translation/conversion occurs.

flux_units = [
    'erg / (s cm2 Angstrom)',
    'ph / (Angstrom s cm2)',
    'ph / (Hz s cm2)', 'ph / (Hz s cm2)'
    ]

sb_units = [
    'erg / (s cm2 Angstrom sr)',
    'ph / (Angstrom s cm2 sr)',
    'ph / (Hz s cm2 sr)', 'ph / (Hz s cm2 sr)'
    ]

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)?

codecov · 2024-08-08T21:44:38Z

Codecov Report

Attention: Patch coverage is 91.66667% with 6 lines in your changes missing coverage. Please review.

Project coverage is 88.81%. Comparing base (3976983) to head (d3ec377).
Report is 1 commits behind head on main.

Files	Patch %	Lines
jdaviz/utils.py	90.19%	5 Missing ⚠️
...specviz/plugins/unit_conversion/unit_conversion.py	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3139      +/-   ##
==========================================
- Coverage   88.82%   88.81%   -0.01%     
==========================================
  Files         112      112              
  Lines       17586    17624      +38     
==========================================
+ Hits        15621    15653      +32     
- Misses       1965     1971       +6

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

pllim · 2024-08-14T18:50:02Z

Patch coverage is only 67.64%. Any chance you can increase that?

rosteen

Works in testing, I left a few comments/suggestions/questions. I also think that the code handling the untranslatable units in utils.py could be reorganized a bit to reduce repeated code, I may make a PR to your branch as a suggestion since it's a bit too much to do in a comment here.

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

jdaviz/core/validunits.py

rosteen · 2024-08-15T14:14:55Z

jdaviz/utils.py

+        else:
+            spectral_values = spec.spectral_axis
+
+        spec_unit = str(spec.flux.unit)


I was going to say this should probably be flux_unit, but I guess the point is that it could be surface brightness so calling it that would also be confusing. How about spec_y_unit?

For this spec unit, it's specifically looking at the Spectrum1D object. We want to see what the native units of the input data are, since that ultimately dictate the conversions/translations that take place. We can't just look at what's displayed along the spectrum viewers y-axis. I added a comment above the line that should clarify that the unit can be flux or sb, but let me know if I should add or explain this better

we just want to avoid confusion with spectral_unit/spectral_axis used in many places that are referring to wavelength/frequency.

Exactly. fluxish_unit? 😆

I didn't realize when I wrote the original comment that this variable is only used once later in the method, so it's not a huge deal.

rosteen · 2024-08-15T14:21:36Z

Patch coverage is only 67.64%. Any chance you can increase that?

I see it at 79.62...would still be nice to increase it a bit if possible.

gibsongreen · 2024-08-15T17:06:30Z

I changed the term 'untranslatable' to 'transitive' for those specific units since they now are translatable. I was also thinking 'indirect', but if there are any other suggestions I can update the name

jdaviz/core/validunits.py

jdaviz/utils.py

jdaviz/tests/test_utils.py

jdaviz/utils.py

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

rosteen

Left a couple more small comments, I think this is close but I want to think about the _transitive_conversion function a little bit more, I think that's where some consolidation could happen but I'm not sure (and it works, so in some sense it doesn't necessarily need to be prettier).

jdaviz/utils.py

pllim · 2024-08-16T12:30:19Z

How is the drop-down order controlled? Jy and MJy is at the top, and the list is not completely alphabetical order, yet two other Jy prefixes are scattered as such:

Should they be grouped together at the top? If so, is it in scope here or a separate ticket?

gibsongreen · 2024-08-16T14:19:42Z

Spectral extraction failed when I tried to grab "mean" instead of "sum". Is fixing this in scope here?

I reproduced and believe I know what the issue is, the spectra if you use min/max/mean are in Surface Brightness units and there needs to be an extra case in _indirect_conversions to handle going from A->D (just as it is done in the image-viewers) and not just A->C.

gibsongreen · 2024-08-16T14:21:34Z

How is the drop-down order controlled? Jy and MJy is at the top, and the list is not completely alphabetical order, yet two other Jy prefixes are scattered as such:
Should they be grouped together at the top? If so, is it in scope here or a separate ticket?

This will also have to be done for the Spectral Unit and the Angle Unit (when fully populated) drop downs. Is the preference here alphabetical order or groupings of units with the same numerator base?

pllim

Mostly minor comments.

Side note: If performance becomes a problem in the future, we can take shortcuts for certain cases because when a /sr converts to another /sr, you don't really need all the fancy indirect stuff. It is basically the same as silently removing /sr, do the normal flux conversion, and then silently reapply it.

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

jdaviz/core/helpers.py

jdaviz/tests/test_utils.py

jdaviz/utils.py

pllim · 2024-08-16T14:29:20Z

Hmm looks like there are merge conflicts now, so needs a rebase too.

Is the preference here alphabetical order or groupings of units with the same numerator base?

Personally, I prefer logical grouping but I cannot speak for everyone.

kecnry · 2024-08-16T14:30:48Z

Can ordering/grouping be a follow-up (and probably not necessary for 4.0)?

…us mouseover global messages

…, test coverage

gibsongreen · 2024-08-16T18:48:50Z

Spectral extraction failed when I tried to grab "mean" instead of "sum". Is fixing this in scope here?

I reproduced and believe I know what the issue is, the spectra if you use min/max/mean are in Surface Brightness units and there needs to be an extra case in _indirect_conversions to handle going from A->D (just as it is done in the image-viewers) and not just A->C.

Took me a bit but I realized since I split the _indirect_conversion into its own function and I was updating orig_units in the new function but that wasn't getting updated in flux_conversion, with that now updating spectral extraction should be working now!

pllim

My remaining unresolved comments are minor and spectral extraction bug seems fixed, so approving. Thanks!

kecnry

Just a few minor comments from looking through the diff, but I think they can be addressed as a follow-up since @gibsongreen is away for a while.

kecnry · 2024-08-19T13:29:32Z

jdaviz/utils.py

+    slice : `astropy.units.Quantity`, optional
+        The current slice of a data cube, with units. Necessary for complex unit
+        conversions/translations that require spectral density equivalencies.


could this be unitless and just documented as required to be in the same units as original_units/values? That would then save a call to app._get_display_units when calling this.

kecnry · 2024-08-19T13:31:34Z

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

+            if self.angle_unit.selected == 'pix':
+                mouseover_unit = self.flux_unit.selected
+            else:
+                mouseover_unit = self.sb_unit_selected


@cshanahan1 - this logic will probably need to change for your effort once images/cubes are guaranteed to always be in SB units

kecnry · 2024-08-19T13:31:53Z

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

+            # if cube was loaded in flux units, we still need to broadcast
+            # a 'sb' message for mouseover info. this should be removed when
+            # unit change messaging is improved and is a temporary fix


FYI @cshanahan1

github-actions bot added cubeviz specviz plugin Label for plugins common to multiple configurations labels Aug 7, 2024

gibsongreen force-pushed the untranslatable_unit_handling branch 2 times, most recently from dd8189c to 14a5b9b Compare August 8, 2024 21:03

gibsongreen added this to the 4.0 milestone Aug 13, 2024

gibsongreen marked this pull request as ready for review August 13, 2024 20:52

gibsongreen requested review from rosteen, javerbukh, pllim, kecnry, bmorris3, haticekaratay and cshanahan1 as code owners August 13, 2024 20:52

rosteen reviewed Aug 15, 2024

View reviewed changes

jdaviz/core/validunits.py Outdated Show resolved Hide resolved

rosteen reviewed Aug 15, 2024

View reviewed changes

jdaviz/core/validunits.py Outdated Show resolved Hide resolved

pllim reviewed Aug 15, 2024

View reviewed changes

jdaviz/utils.py Outdated Show resolved Hide resolved

pllim reviewed Aug 15, 2024

View reviewed changes

jdaviz/tests/test_utils.py Show resolved Hide resolved

pllim reviewed Aug 15, 2024

View reviewed changes

jdaviz/utils.py Show resolved Hide resolved

gibsongreen mentioned this pull request Aug 15, 2024

Update messaging from unit conversion plugin #3155

Merged

pllim reviewed Aug 15, 2024

View reviewed changes

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

rosteen reviewed Aug 15, 2024

View reviewed changes

jdaviz/utils.py Outdated Show resolved Hide resolved

jdaviz/utils.py Outdated Show resolved Hide resolved

jdaviz/utils.py Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

pllim reviewed Aug 16, 2024

View reviewed changes

gibsongreen added 13 commits August 16, 2024 11:29

re-add untranslatable units, add translation equivalencies

93dfeeb

readd untranslatable units to app defined units

aac6d69

fix rebase, enable untranslatable units, testing and coords needs work

1133638

fix styling

d9f8fea

resolve spectrum-viewer mouseover, image-viewers need work

6058671

generalize flux_conversion, enable untranslatable units, temporarily …

8f07658

…us mouseover global messages

change log entry, update test_utils.py tests

2c0a3b4

increase test coverage

e066482

seperate flux conversion and transitive units, remove duplicate units…

89d1c87

…, test coverage

remove duplicate/reversed units

03736db

implement commented changes

43272fd

update tests

12fb37c

address review comments, begin work on spectral extraction issue

ea76c26

gibsongreen force-pushed the untranslatable_unit_handling branch from e14f4c1 to ea76c26 Compare August 16, 2024 16:22

resolve failing tests and spectral extraction issue

d3ec377

pllim approved these changes Aug 16, 2024

View reviewed changes

pllim added the Ready for final review label Aug 16, 2024

kecnry reviewed Aug 19, 2024

View reviewed changes

kecnry merged commit 95d6008 into spacetelescope:main Aug 19, 2024
25 checks passed

rosteen mentioned this pull request Aug 20, 2024

Skip flux conversion for moment maps #3158

Merged

This was referenced Aug 30, 2024

convert flux cubes to per-square-pixel surface brightness cubes #3156

Merged

BUG: Filter out impossible SB <> SB and Flux <> Flux conversions in unit conversion plugin #3183

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Untranslatable unit handling #3139

Untranslatable unit handling #3139

gibsongreen commented Aug 7, 2024 •

edited

Loading

codecov bot commented Aug 8, 2024 •

edited

Loading

pllim commented Aug 14, 2024

rosteen left a comment

rosteen Aug 15, 2024

gibsongreen Aug 15, 2024

kecnry Aug 15, 2024

rosteen Aug 15, 2024

rosteen commented Aug 15, 2024

gibsongreen commented Aug 15, 2024

rosteen left a comment

pllim commented Aug 16, 2024

This comment was marked as resolved.

gibsongreen commented Aug 16, 2024

gibsongreen commented Aug 16, 2024

pllim left a comment •

edited

Loading

pllim commented Aug 16, 2024

kecnry commented Aug 16, 2024

gibsongreen commented Aug 16, 2024 •

edited

Loading

pllim left a comment

kecnry left a comment

kecnry Aug 19, 2024

kecnry Aug 19, 2024

kecnry Aug 19, 2024

Untranslatable unit handling #3139

Untranslatable unit handling #3139

Conversation

gibsongreen commented Aug 7, 2024 • edited Loading

Description

Change log entry

Checklist for package maintainer(s)

codecov bot commented Aug 8, 2024 • edited Loading

Codecov Report

pllim commented Aug 14, 2024

rosteen left a comment

Choose a reason for hiding this comment

rosteen Aug 15, 2024

Choose a reason for hiding this comment

gibsongreen Aug 15, 2024

Choose a reason for hiding this comment

kecnry Aug 15, 2024

Choose a reason for hiding this comment

rosteen Aug 15, 2024

Choose a reason for hiding this comment

rosteen commented Aug 15, 2024

gibsongreen commented Aug 15, 2024

rosteen left a comment

Choose a reason for hiding this comment

pllim commented Aug 16, 2024

This comment was marked as resolved.

gibsongreen commented Aug 16, 2024

gibsongreen commented Aug 16, 2024

pllim left a comment • edited Loading

Choose a reason for hiding this comment

pllim commented Aug 16, 2024

kecnry commented Aug 16, 2024

gibsongreen commented Aug 16, 2024 • edited Loading

pllim left a comment

Choose a reason for hiding this comment

kecnry left a comment

Choose a reason for hiding this comment

kecnry Aug 19, 2024

Choose a reason for hiding this comment

kecnry Aug 19, 2024

Choose a reason for hiding this comment

kecnry Aug 19, 2024

Choose a reason for hiding this comment

gibsongreen commented Aug 7, 2024 •

edited

Loading

codecov bot commented Aug 8, 2024 •

edited

Loading

pllim left a comment •

edited

Loading

gibsongreen commented Aug 16, 2024 •

edited

Loading