Repository navigation
Fixes to long-running issues in fitting example - #9
Conversation
…same code to assign events to bins.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #9 +/- ##
==========================================
- Coverage 79.37% 78.87% -0.50%
==========================================
Files 6 6
Lines 921 923 +2
==========================================
- Hits 731 728 -3
- Misses 190 195 +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
Plot bin indices currently wrap invalid values and can silently display the wrong fit.
Review effort: Balanced
Findings: 1
Open (1)
What changed in this PR
Updates PSF fitting plots to show physical per-steradian densities and aligns event-bin assignment between fitting and likelihood evaluation.
Changes:
- Adds shared edge-based, clamped bin lookup.
- Reworks fit plots with PDF densities and bin averages.
- Adds regression tests for bin assignment and repeated percentile values.
| File | Description |
|---|---|
kingmaker/utils.py |
Adds shared bin-index helper. |
kingmaker/fitting.py |
Uses shared binning and revises plotting. |
kingmaker/wrapper.py |
Uses edge-based event-bin lookup. |
tests/test_utils.py |
Tests bin lookup boundaries and clamping. |
tests/test_fitting.py |
Tests repeated-value percentile bins. |
tests/test_wrapper.py |
Tests unequal-width bin assignment. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| bin_indices = tuple( | ||
| int(i) % n for i, n in zip(bin_indices, self.parametrization_shape, strict=True) | ||
| ) |
Bennett pointed out that there was something wonky in the fitting example code regarding the plotting. This turned out to be due to a rescaling of the PDF to match the peak of the distribution. This was fixed with a rewrite: we're now just plotting the per-sterradian PDF directly instead of trying to screw around with scaling factors. This works pretty well and we can make reasonable looking plots now.
Along the way, there was also some simplification to make sure the fitting and likelihood code use the same method to assign events to bins.the same code to assign events to bins.