Repository navigation
Fixes for signal subtraction - #10
Conversation
…s. Fix that and update the docs. Double check the masking for the signal subtraction.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #10 +/- ##
==========================================
- Coverage 78.87% 78.85% -0.02%
==========================================
Files 6 6
Lines 923 946 +23
==========================================
+ Hits 728 746 +18
- Misses 195 200 +5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Public README, examples, and notebook references still describe the obsolete RA-integrated quantity.
Review effort: Balanced
Findings: 1
What changed in this PR
Corrects signal-subtraction PDFs to use RA-averaged, per-steradian densities.
Changes:
- Applies the correct RA-average normalization and optimized integration bounds.
- Expands interpolation bounds and clamps out-of-grid parameters.
- Adds normalization, accuracy, and clamping tests.
| File | Description |
|---|---|
kingmaker/utils.py |
Computes RA-averaged grids. |
kingmaker/pdf.py |
Updates interpolation, defaults, and terminology. |
kingmaker/wrapper.py |
Corrects API documentation. |
tests/test_king_pdf.py |
Tests averaging and clamping. |
docs/signal_subtraction.rst |
Documents the corrected normalization. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if np.any((params < lower) | (params > upper)): | ||
| warnings.warn( | ||
| "alpha or beta outside the marginalization grid; clamping to grid edges. " | ||
| "Widen points_alpha or points_beta to avoid this.", | ||
| RuntimeWarning, | ||
| stacklevel=3, | ||
| ) | ||
| np.clip(params, lower, upper, out=params) |
There was a problem hiding this comment.
🟡 Changes recommended
Clamping currently accepts mathematically invalid King parameters instead of rejecting them.
3 open findings
1 resolved since last review
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
There was a problem hiding this comment.
🟢 Approval recommended
The implementation, numerical tests, units, and documentation consistently reflect the corrected RA-averaged formulation.
2 open findings
1 resolved since last review
🧠 Review effort: Balanced
Give feedback about Copilot approvals in this survey to enter a drawing for a $150 gift card.
Apparently my understanding of csky's signal subtraction was wrong. I thought it needed the RA-integrated PDF renormalized in sin(dec). Apparently it wants the RA-averaged PDF? So update the code to do that and fix all of the many, many references...
Edit: Ah, right. It's a unit thing. The integrated version has different units than the standard PDFs, so we have to use the average here to get back to 1/sr.