jdaviz icon indicating copy to clipboard operation
jdaviz copied to clipboard

First pass at enabling per steradian unit conversion

Open javerbukh opened this issue 1 year ago • 2 comments

Description

This pull request is part of the cubeviz spectral extraction units hack day. The goal for this PR is to enable flux unit conversion for surface brightness units. The way this PR does that is by removing the per steradian, finding equivalent units, and then adding the per steradian back to the unit.

Some follow-up work:

  • [ ] Slice indicator does not appear correctly with certain flux units per steradian
  • [ ] Propagate display unit changes to entire cubeviz helper so that mouse over units and other plugins can use those units
  • [ ] Cursor does not appear over collapsed spatial subset in spectrum viewer
  • [ ] Need a better check for presence of steradian than if 'sr' in units

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.
  • [ ] After merge, any internal documentations need updating (e.g., JIRA, Innerspace)?

javerbukh avatar Feb 13 '24 16:02 javerbukh

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 90.89%. Comparing base (fb6d841) to head (bf99071). Report is 47 commits behind head on main.

:exclamation: Current head bf99071 differs from pull request most recent head 86707df. Consider uploading reports for the commit 86707df to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2702      +/-   ##
==========================================
+ Coverage   90.83%   90.89%   +0.06%     
==========================================
  Files         163      163              
  Lines       21502    21545      +43     
==========================================
+ Hits        19532    19584      +52     
+ Misses       1970     1961       -9     

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

codecov[bot] avatar Feb 13 '24 21:02 codecov[bot]

@javerbukh - now that the different PRs have been merged in glue-core and glue-jupyter, could you try again before I go ahead and do a release?

astrofrog avatar Apr 23 '24 13:04 astrofrog

Closing this, superseded by #3149 and others.

rosteen avatar Aug 20 '24 13:08 rosteen