-
-
Notifications
You must be signed in to change notification settings - Fork 41
Horne extraction error propagation #296
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #296 +/- ##
==========================================
+ Coverage 89.76% 89.90% +0.13%
==========================================
Files 17 17
Lines 1817 1822 +5
==========================================
+ Hits 1631 1638 +7
+ Misses 186 184 -2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
tepickering
left a comment
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.
thanks for adding this! looks good except for one case that isn't covered by the new tests according to codecov. i would add that and then feel free to merge.
specreduce/extract.py
Outdated
| output_uncertainty = InverseVariance(1.0 / extracted_variance / unit**2) | ||
| else: | ||
| # Fallback to VarianceUncertainty for unknown types | ||
| output_uncertainty = VarianceUncertainty(extracted_variance * unit**2) |
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 looks like coverage for this case is missing from the tests you added.
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.
Fixed! The whole uncertainty type conversion could be simplified to three lines.
|
well that's exciting. the |
…ing against an analytical calculation for the expected variance.
56623a0 to
1ab2f37
Compare
This PR adds uncertainty propagation to HorneExtract so that uncertainties from 2D input images are properly propagated to the extracted 1D spectrum. Turns out this was very straightforward since we're already calculating the uncertainty for the Horne extraction itself (it's
1/denwithout the normalisation). So, aside from handling uncertainty types and such, we actually needed to add one extra line of code... I also renamedimgtofluxso that the variable is not mixed withimage, and so that the variable naming is more understandable at the end of the method.