Fix unit conversion from flux to surface brightness

[astrofrog]

Description

While looking at issues with limits not updating when changing to certain units, I spotted an unrelated issue. In to_unit, the units on the original data should be irrelevant, and if glue asks for a conversion from e.g. Jy to MJy, we should ignore the original units on the data, since they might already have been previously converted to Jy which is why glue is asking for Jy to MJy. So looking at spec.unit.bases is not the right thing to do. A lot of the change here is indentation so be sure to check the diff ignoring whitespace.

I've expanded the test for to_unit.

There are other issues with the to_unit code as written currently, but I thought I'd break it up into conceptual chunks.

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

[gibsongreen]

My reasoning for having the additional conditional statements for checking the original units on the data is because the data item from the data collection does not update (nor the units). The numpy array that makes up values in to_unit(), which is the array of values that get converted, come from the data item in the data collection, not what is currently displayed in the profile-viewer? If the data item doesn't update, and these are the values that are converted, each conversion or translation is starting from the original values of the data and the original units.

I did test this PR locally and the behavior does seem to function as expected. I did double check to see what happens to values once multiple conversions or translations occur (and it does match the original values of the data item). So I assume I am overlooking something? I will approve, but do you see what I am missing here that makes this updated logic work?

[astrofrog]

The numpy array that makes up values in to_unit(), which is the array of values that get converted, come from the data item in the data collection, not what is currently displayed in the profile-viewer

This is not always true - in particular in the case of the min/max limits, glue will take the existing limits and convert them to the native data units and then to new units - which means that for one of the calls to to_unit, values will not be in the 'native' data units.

[gibsongreen]

Functionality is maintained with these changes, while simplifying to_unit(). Bearing additional changes in #2889 and #2890, discussed in tagup, the checks for the original units of the data item should not be needed post these 2 PRs.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.69%. Comparing base (50ebc1e) to head (d19909a).
Report is 180 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2888      +/-   ##
==========================================
- Coverage   88.70%   88.69%   -0.01%     
==========================================
  Files         111      111              
  Lines       17125    17122       -3     
==========================================
- Hits        15190    15187       -3     
  Misses       1935     1935              

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

[gibsongreen]

@kecnry per this comment related to CI, they are the 2 same failures here so I will also merge: #2897 (comment)

[pllim]

@astrofrog , this actually added 9 new failures to the devdeps failures galore. See https://github.com/spacetelescope/jdaviz/actions/runs/9292738683/job/25574299461

Ignore these 2:

• jdaviz/core/tests/test_tools.py::test_stretch_bounds_and_spline
• jdaviz/configs/imviz/tests/test_simple_aper_phot.py::test_annulus_background

[astrofrog]

@pllim - ok I'll take a look at these failures

[astrofrog]

@pllim - are you sure these aren't due to astropy/specutils#1141?

[astrofrog]

Yes the errors:

E       astropy.units.core.UnitConversionError: 'Jy m' (force) and 'Jy' (spectral flux density) are not convertible

seem to be related to astropy/specutils#1141 (cc @rosteen)

[pllim]

BTW yeah, not caused by this PR after all. Sorry for the noise! #2905