-
Notifications
You must be signed in to change notification settings - Fork 76
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
expose surface brightness units in internal get_display_units method #3112
Conversation
66b50ff
to
b1968b5
Compare
Thanks for enabling the selection of parameters in the method. Are failing tests related to this PR? |
Yep, I just also replied in #3107 (comment) that the default casting is causing issues with tests, so I may split that part out to a follow-up effort so that it doesn't block the review of the rest of this. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good to me. maybe a small test to make sure future changes don't break this since a lot of the plugins will be using this? i'll update this function when the non-steradian surface brightness unit support is finished.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Outside of the python 3.11 failure everything looks good to me, love the convenience this adds. We might want to make a note that in generalizing the square angle that this will be an additional area to account for when we support more then sr
.
f8a3759
to
4afd708
Compare
hmmm I thought this was passing tests before, but still doesn't on a rebase. I'll have to try to track down and fix before merging - if its anything major, I'll ask for re-reviews. |
4afd708
to
9fb99d7
Compare
passes now on a rebase without any changes 🤷♂️ so I'll merge! |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3112 +/- ##
==========================================
- Coverage 88.85% 88.85% -0.01%
==========================================
Files 112 112
Lines 17396 17409 +13
==========================================
+ Hits 15458 15468 +10
- Misses 1938 1941 +3 ☔ View full report in Codecov by Sentry. |
Description
This pull request exposes surface brightness units (in addition to flux units) in
app._get_display_units
. This also adds support for requestingapp._get_display_units('spectral_y')
which will return either flux or SB, depending on which is being use on the y-axis of the spectral viewer. This is intended for internal use only, but could be useful for WIPs developing support in plugins to react to a change in display units.Previously
flux
,sb
, andspectral
were supported as long as the plugin existed. This PR adds a fallback in the case where the plugin does not exist and also adds support forspectral_y
to read from theflux_or_sb
switch and then retrieve the appropriate unit.Change log entry
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, maintainershould 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.
trivial
label.