Untranslatable unit handling

gibsongreen commented 1 month ago

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

[x] 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.

[x] 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[bot] commented 1 month ago

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 :warning:
...specviz/plugins/unit_conversion/unit_conversion.py	85.71%	1 Missing :warning:

Additional details and impacted files

```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

pllim commented 1 month ago

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

rosteen commented 1 month ago

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 commented 1 month ago

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

pllim commented 1 month ago

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?

pllim commented 1 month ago

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

gibsongreen commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

gibsongreen commented 1 month ago

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!

spacetelescope / jdaviz