Surface brightness -> solid angle conversion updates

[gibsongreen]

Description

This pull request is to address:

• The surface brightness drop down will become read-only text
• A new drop down will be created for the solid angle unit
• Surface brightness will update when changes to flux or the angle are made
• the “flux or sb” dropdown will no longer update automatically when making a selection to the flux/angle/sb unit drop downs

To be done:

• compatibility with Specviz (flux data, surface brightness data doesn't populate (sb/flux) NEEDS UPDATE AFTER Update the mouse over units by listening to the unit-conversion plugin #2931)
• fix limits issue in Specviz NEEDS UPDATE AFTER Update the mouse over units by listening to the unit-conversion plugin #2931
• untranslatable units compatibility
• pass existing tests

Untitled1.mov

Currently, we enable the selection of the surface brightness unit drop down. This ticket will make the surface brightness drop down read-only. A new dropdown will be added, for selecting the denominator, the square angle unit. Surface brightness will then be calculated from the selected flux and angle unit.

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

[codecov]

Codecov Report

Attention: Patch coverage is 93.65079% with 4 lines in your changes missing coverage. Please review.

Project coverage is 88.89%. Comparing base (a126296) to head (96b9263).
Report is 8 commits behind head on main.

Files	Patch %	Lines
jdaviz/core/validunits.py	69.23%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3111      +/-   ##
==========================================
- Coverage   88.93%   88.89%   -0.04%     
==========================================
  Files         112      112              
  Lines       17411    17385      -26     
==========================================
- Hits        15485    15455      -30     
- Misses       1926     1930       +4     

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

[rosteen]

I think this looks pretty good, didn't find any problems testing it. I left one suggestion, I think I can approve after that's addressed.

[rosteen]

Suggested change

	dc_unit = self.app.data_collection[0].get_object().flux.unit
	dc_unit = self.app.data_collection[0].get_component("flux").units

I think this is probably a little faster than going through the glue-astronomy translator to create a Spectrum1D object just to get the flux.

[gibsongreen]

Awesome! I will make the change now, I have this spot that does the same thing from my last PR so I will make the change there too (EDIT: there is actually place where this is done in unit_conversion.py:
#2940 (comment)

and I am going to make this same adjustment of moving the returns outside of conditionals:
#2940 (comment)

[cshanahan1]

testing this out now and is working well

it looks like app._get_display_unit('sb') is no longer available - i know you can use flux and angle to get this unit, but since other plugins are using this would it make sense to keep this way of accessing it and just make sure everything stays in sync?

[rosteen]

One small suggestion on a comment, and we spoke offline about some variable name/conditional cleanup, so I'll hold off on approving for you to make that change. But testing in the notebook, everything works!

[rosteen]

I think this looks good now - there are a couple things I noticed that I think were from the previous PR, not changed here, so I might open a quick follow up after this is merged. But no need to hold this up since this PR didn't introduce them.

[rosteen]

it looks like app._get_display_unit('sb') is no longer available

@cshanahan1 It doesn't look like this PR changed app.py, any idea when this changed?

[cshanahan1]

it looks like app._get_display_unit('sb') is no longer available

@cshanahan1 It doesn't look like this PR changed app.py, any idea when this changed?

i think its because its trying to access the 'sb_unit_selected' from the unit conversion plugin, which now fails

[gibsongreen]

it looks like app._get_display_unit('sb') is no longer available

@cshanahan1 It doesn't look like this PR changed app.py, any idea when this changed?

i think its because its trying to access the 'sb_unit_selected' from the unit conversion plugin, which now fails

It comes from removing sb_unit_selected in this PR, below is what gets called when you do this

app._get_display_unit('sb')

   return getattr(self._jdaviz_helper.plugins.get('Unit Conversion')._obj,
                       f'{axis}_unit_selected')

In unit_conversion.py I can change the traitlet from self.sb_unit (new to this PR) to self.sb_unit_selected and it will function as it did in the past, any opposition to this change? That or changing the fstring to remove the selected portion but that might have implications I'm unaware of without testing

[rosteen]

Ahh I see, good call - I didn't read to the end of the method 😅. I agree it might be worth adding a manual case for sb in get_angle_unit that combines the results for flux_unit and angle_unit automatically.

[rosteen]

In unit_conversion.py I can change the traitlet from self.sb_unit (new to this PR) to self.sb_unit_selected and it will function as it did in the past

This sounds simpler than what I just suggested.

[gibsongreen]

In unit_conversion.py I can change the traitlet from self.sb_unit (new to this PR) to self.sb_unit_selected and it will function as it did in the past

This sounds simpler than what I just suggested.

@cshanahan1 let me know if this suffices, or if the other suggested change is better for the generalization of units ticket I candy that instead

[rosteen]

That change works for me, my approval stands 🙂

[cshanahan1]

yep that works for me now, thanks!