-
-
Notifications
You must be signed in to change notification settings - Fork 128
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
Fix moment calculations #1141
Fix moment calculations #1141
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1141 +/- ##
=======================================
Coverage 86.82% 86.83%
=======================================
Files 63 63
Lines 4525 4528 +3
=======================================
+ Hits 3929 3932 +3
Misses 596 596 ☔ View full report in Codecov by Sentry. |
Actually, the higher orders also need a similar correction. The sum should have the terms multiplied by |
There's an unrelated deprecation warning in devdeps that I'll address separately. |
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.
It is now correct in the 1D case for flux density and it seems to propagate well in Cubeviz. Thank you!
Ah, FWIW this is upstream in GWCS: spacetelescope/gwcs#499. |
Looks like this broke tests over at Jdaviz, see spacetelescope/jdaviz#2888 (comment) |
This solves a bug that was reported offline by @camipacifici - moment 0 should be in units of flux * spectral axis, being the flux integrated over the spectral range, matching the units of line flux. This PR changes from doing a simple sum of flux for moment 0, to summing flux*dx as in
line_flux
.The case for
SpectrumCollection
is because thespectral_axis
of aSpectrumCollection
comes out asSpectralCoord
, and we needbin_edges
fromSpectralAxis
in each individual member of the collection.