-
-
Notifications
You must be signed in to change notification settings - Fork 42
Boxcar extraction error propagation #295
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
… various uncertainty types (`VarianceUncertainty`, `StdDevUncertainty`, `InverseVariance`), and added associated unit tests.
…t` and added more unit tests.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #295 +/- ##
==========================================
+ Coverage 89.67% 89.76% +0.09%
==========================================
Files 17 17
Lines 1801 1817 +16
==========================================
+ Hits 1615 1631 +16
Misses 186 186 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
it looks like this and #296 have overlapping changes. at first glance it looks like #296 is based off the changes here which means either this should be merged first or just ignore this one and do everything in #296. it also looks like this and #296 mostly supercedes #286. we will want to reconcile that and see what we can still incorporate from there. |
|
The two PRs don't have overlapping changes, even though they both change the same files. I revised the Boxcar extraction first and decided to revise the Horne extraction in a separate PR to keep the PRs easy to review. However, this was before I realised how minimal the changes to the Horne extraction would be... I can combine the Horne extraction PR with this one if you prefer, or we can merge the two PRs separately. And yes, these two supersede #286. I've been looking into the PR and think we can probably incorporate at least some of the tests. However, I'd prefer to see what we can do here after we have merged these two PRs. |
- Cleaned up the related unit tests a bit.
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.
my bad not looking close enough to see the changes were actually not overlapping. the way github displayed the diffs made them look so similar. which implies there may be some redundancy that could be refactored out at some point. at any rate, these changes are good to go so merge at will.
This PR adds uncertainty propagation to BoxcarExtract so that uncertainties from 2D input images are properly propagated to the extracted 1D spectrum. The approach supports
VarianceUncertainty,StdDevUncertainty, andInverseVarianceuncertainty types, and the PR includes a full set of tests for the functionality.