-
Notifications
You must be signed in to change notification settings - Fork 75
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
model fitting: ensure specutils has access to equivs & set targ solid angle #3343
model fitting: ensure specutils has access to equivs & set targ solid angle #3343
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3343 +/- ##
==========================================
- Coverage 88.80% 88.78% -0.03%
==========================================
Files 125 125
Lines 19137 19174 +37
==========================================
+ Hits 16995 17023 +28
- Misses 2142 2151 +9 ☔ View full report in Codecov by Sentry. |
Works for me, thank you! |
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.
Good catch, I didn't clock this as a unit conversion when I was working on unit conversions for model fitting!
I do wonder if this will ever encounter one of the special cases where a spectral_density AND a pix2 equivalency are needed, and if with_flux_unit will handle that. Have you tested anything like that at all?
The The manga cube in issue #3342 is in the |
pix2_in_flux = 'pix2' in spec.flux.unit.to_string() | ||
pix2_in_sb = 'pix2' in sb_unit | ||
# Handle various cases when PIX2 angle unit is present in the conversion | ||
if pix2_in_flux and pix2_in_sb: | ||
spec = spec.with_flux_unit(u.Unit(spec.flux.unit)*PIX2, equivalencies=equivalencies) # noqa | ||
spec = spec.with_flux_unit(u.Unit(sb_unit)*PIX2, equivalencies=equivalencies) | ||
elif pix2_in_flux: | ||
spec = spec.with_flux_unit(u.Unit(spec.flux.unit) * PIX2, equivalencies=equivalencies) # noqa | ||
elif pix2_in_sb: | ||
spec = spec.with_flux_unit(u.Unit(spec.flux.unit) / PIX2, equivalencies=equivalencies) # noqa | ||
|
||
spec = spec.with_flux_unit(sb_unit, equivalencies=equivalencies) |
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.
Maybe in a small effort before flux_conversion
and flux_conversion_general
, these both right now return the converted values
as an array or spectral_axis
as a Quantity object. It'd be worth adding the ability to return the spec
Spectrum1D object as well and then we can update this logic block.
@cshanahan1 if you test with this cube:
cubeviz.load_data(f"https://data.sdss.org/sas/dr17/manga/spectro/redux/v3_1_1/7443/stack/manga-7443-12703-LOGCUBE.fits.gz")
and convert the flux_unit to Jy, this no longer throughs a UnitConversionError for the case you brought up. Test coverage is there now too!
This PR is a blocker for a notebook we need merged once 4.1 is released next week, so I will create a ticket and start working on the Spectrum1D compatibility when converting flux.
This draft PR resolves:
spec.with_flux_unit
did not have access topix2
custom unit equivalenciesTo reproduce on main:
solid_angle_in_targ
was not being set aftertarg_units
was update to include a solid angle unit. Setting 'targ_units' triggered subsequent indirect conversion logic. By setting thesolid_angle_in_targ
we follow the same logic route and ensure we maintain output units in surface brightness.To reproduce:
Description
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.