get_data(use_display_units=True) to respect flux or sb selection

[kecnry]

Description

This pull request fixes viz.get_data(use_display_units=True) on a 1d spectrum to respect the user choice of surface brightness or flux units. This then fixes the previews in line-analysis, which are calling get_data(use_display_units=True) under-the-hood when computing the continuum.

Screen.Recording.2024-08-02.at.10.24.59.AM.mov

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]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.82%. Comparing base (b3bbd9f) to head (8a129d0).
Report is 138 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3129      +/-   ##
==========================================
- Coverage   88.85%   88.82%   -0.03%     
==========================================
  Files         112      112              
  Lines       17409    17409              
==========================================
- Hits        15468    15464       -4     
- Misses       1941     1945       +4     

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

[kecnry]

@gibsongreen - the first line works (although is inconsistent with the check above which is expecting /sr), and the second line is then failing the assertion by returning in MJy/sr. Is this something going wrong in flux_conversion or do these need to wait for non-per-sr support?

[gibsongreen]

I didn't change any of the extraction code to append u.pix before it gets added to the data collection, so the data item for 'Spectrum (sum)' is in MJy (without a denominator). The test failure I see running it locally is a conversion error between MJy / pix and MJy (which we don't have supported yet with the flux_conversion). The per pixel support right now is only when data loaded in is flux units, and I didn't add full support for it, that goal of #3111 was predicated on support for u.sr (with any other denominator being added in the generalization, and switching denominators possibly in that PR or in a future one).

The y_display_unit test on L166 I believe is actually correct and the data displayed is per sr. There is this assumption in #3111 that the we don't allow change of the angle_unit value. Below is the line where this assumption is made. This support was going to be in the generalization ticket, and since this is the case, when you translate, you still get .pix in this case which for the spectral_y test on L174.

https://github.com/gibsongreen/jdaviz/blob/96b92634efda8551cc83624cb2af7d36b0543fba/jdaviz/configs/specviz/plugins/unit_conversion/unit_conversion.py#L177

[kecnry]

@cshanahan1 - we now updated this test to have the input cube be in MJy/sr and then the rest of the logic falls into place. Would your future PR (adding support for non-steradian angle units) be able to re-introduce a test-case with input in flux units, test that they are coerced to flux/pix**2, and then test any of this similar logic that you think needs extra coverage?

[javerbukh]

The code looks reasonable and I could not trigger any tracebacks, so looks good to me! I think it would be worthwhile to have someone check the science implications, however.

[gibsongreen]

Everything looks good to me!