Repository navigation
feat: init binned spectrum and add axis conversions - #61
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #61 +/- ##
==========================================
+ Coverage 95.00% 96.05% +1.05%
==========================================
Files 9 11 +2
Lines 180 228 +48
==========================================
+ Hits 171 219 +48
Misses 9 9 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
This is a really nice PR, thanks @jamesrafe! I think this would be a good opportunity to flex our ecosystem here and start factoring out some common functionality I've been seeing pop-up in a few of our packages. For starters: @cgarling, what would you think about upstreaming your spectral density conversions in PhotometricFilters.jl to UnitfulEquivalences.jl? I've opened a PR to demonstrate what that might look like sostock/UnitfulEquivalences.jl#31 Next, I think we could replace the custom uconvert(u"THz", spec) # Just convert the spectral axis
uconvert((u"keV", u"W/m^2/keV"), spec) # Convert the spectral AND flux axiscompared to the separate There's also some cleanup in the binned spectrum logic I think we could apply here. This will all be included in my review, but just wanted to give y'all a heads up first on where I was thinking of taking this |
|
No problem with moving the spectral density conversions if they accept the pr, IMO its a little more specialized than their other equivalences so IDK if they will want to add it |
|
I may have been spoiled with the DynamicQuantities dev being so receptive to some of our astro-specific PRs in the past, so will see I guess 😅 |
|
Ok, I think the bones are in place now. Instead of a separate review at this stage, I've just added a PR description to summarize where things are at. It depends on one moving target, the potential PR to UnitfulEquivalences.jl for our unit conversions if they are amenable, but I think the other main piece of this PR, the addition of Please have a look all, and feel free to make any additional suggestions if you have any at this time. I know I personally want to polish up the plotting bits, but I think that can be saved for a future PR. |
|
Thanks a lot, @icweaver! Thank you for taking the time to work through the code haha. The clean-up of the BinnedSpectrum logic and the extra changes to align with the new type has been amazing. I've learned a lot from your changes. The PR description is great, as well! I should have made one beforehand. |
fjebaker
left a comment
There was a problem hiding this comment.
This is really nice stuff, thanks @jamesrafe and @icweaver for all your work!
A quick thought: prohibiting reddening and de-reddening via dust extinction for BinnedSpectrum makes perfect sense in this context, but perhaps we should also consider adding more naive (i.e. cosmological redshift type) reddening too. That is, something that accepts a redshift z and shifts the spectral axis appropriately. This would work for binned spectra too, as it boils down to a single multiplication.
There's application in this especially for simulated spectra.
|
sgtm! I think we already have this in place (?) with |
|
Ah I missed |
|
Sweet, we were able to get |
Adds a
BinnedSpectrumtype and unit-aware spectral axis conversions.BinnedSpectrumA new$m \times 2$ matrix of bin edges and the flux axis is an $m$ -length vector.
Spectrumkind for binned data (e.g., X-ray spectra): the spectral axis is anspectrum()for both unitless and unitful inputs, with validation: exactly two edge columns, and each row strictly ordered in the direction of the spectral axis (catches swapped, mixed-order, and zero-width edges; descending axes are supported).spec[i]for anyInteger, ranges, masks), iteration, and display work like the other spectrum kinds;redshift/doppler_shifthandle bin edges.redden/dereddenreject binned spectra with an explicit error (extinction laws need a bin-integration convention) instead of failing inside DustExtinction.Axis conversions
Unitful.uconvertis extended for spectra, converting between wavelength, frequency, and photon energy via the photon equivalenceSpectralDensity(at)equivalence from UnitfulEquivalences.jl internally, which preserves integrals:redshift/doppler_shiftnow also handle frequency and energy axes (compressing instead of stretching).