-
Notifications
You must be signed in to change notification settings - Fork 37
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
HLA-1161: MagErrAp2 and other magnitude errors in HAP catalogs have incorrect negative values #1700
HLA-1161: MagErrAp2 and other magnitude errors in HAP catalogs have incorrect negative values #1700
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1700 +/- ##
==========================================
- Coverage 33.78% 33.78% -0.01%
==========================================
Files 126 126
Lines 31169 31172 +3
Branches 5715 5715
==========================================
+ Hits 10530 10531 +1
- Misses 19450 19451 +1
- Partials 1189 1190 +1 ☔ View full report in Codecov by Sentry. |
Had to change the number of expected sources in the point source catalog for test_svm_segment_total_cat() in test_svm_ibqk07.py. Currently investigating whether that is linked to these changes. |
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.
I left a couple minor comments inline. I'm not familiar with the code but reading the discussion in the ticket, the changes here address that so I'm approving the PR.
Co-authored-by: Nadia Dencheva <[email protected]>
Resolves HLA-1161
This PR addresses an issue (#1704) where (1) magnitude errors are being calculated as negative numbers, and (2) these are used for threshold calculations resulting in bad sources with good flags.
With this code we switch to calculating the good_snr threshold based on the flux error (which is always positive). We also fix the negative magnitude error issue by setting the magnitude error to infinity is the flux (and consequently the magnitude) is less than zero.
I had to change the number of expected sources for one of the regression tests because it no longer recognizes two sources with magnitudes and fluxes = -9999.
I also added dates to our new Architecture Decision Record (ADR) document.
Checklist for maintainers
CHANGELOG.rst
within the relevant release sectionHow to run regression tests on a PR
Jenkins Test (Dev build failing for Numpy 2.0 issue)