Fit the CDF with scipy.optimize.minimize#6
Merged
Conversation
…ze instead with a numerically calculated gradient and skip the grid search.
Welcome to Codecov 🎉Once you merge this PR into your default branch, you're all set! Codecov will compare coverage reports and display results in all future pull requests. Thanks for integrating Codecov - We've got you covered ☂️ |
There was a problem hiding this comment.
Pull request overview
This PR updates the King PSF parameter fitting approach to improve numerical stability by fitting against the CDF (instead of the PDF) and switching the optimizer to scipy.optimize.minimize, while also exposing weight-outlier handling controls through the higher-level wrapper.
Changes:
- Replaced PDF least-squares fitting with CDF-based objective minimized via L-BFGS-B (finite-difference gradient).
- Removed the prior multi-start grid-search/least-squares helper in favor of a smaller set of seeded minimizations.
- Exposed
remove_weight_outliersandweight_outlier_percentilesinKingSpatialLikelihoodand forwarded them toKingPSFFitter.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
kingmaker/wrapper.py |
Adds configuration knobs for weight outlier trimming and passes them into the fitter. |
kingmaker/fitting.py |
Reworks the per-bin fitting routine to optimize a CDF-based objective with minimize and updates histogram storage scaling. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…to speed up fitting and normalizations. Since normalization is faster, swap fitter and likelihood to use KingPDF instead of InterpolatedKingPDF, which wil be removed.
…ling with reshaping errors. That comes next.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 16 out of 17 changed files in this pull request and generated 15 comments.
Comments suppressed due to low confidence (1)
kingmaker/fitting.py:407
self.dpsi_bins[param_idx]is written before checkinglen(dpsi_bins) < 3and before the fit succeeds. If the bin is later skipped or optimization fails, this leaves partially-updated per-bin diagnostics that look valid downstream. Consider only writingdpsi_bins/histograms/uncertaintiesafter a successful fit (or explicitly clearing them on failure).
# Create bins for this subset. Also calculate the
# phase space parameter while we're here. We'll need
# it in order to store the histograms as PDFs later.
dpsi_bins = self._get_percentile_bins(self.dpsi_nbins, masked_dpsi, masked_weights)
dpsi_bins = np.unique(dpsi_bins)
delta = -2 * np.pi * np.diff(np.cos(dpsi_bins))
self.dpsi_bins[param_idx][: len(dpsi_bins)] = dpsi_bins
# If we don't have enough bins, skip
if len(dpsi_bins) < 3:
return False
|
|
||
| ``` | ||
| f(θ | α, β) ∝ [1 + (θ/α)² / (2β)]^(-β) | ||
| f(theta | alpha, beta) = [1 + (1 - cos(theta)) / (alpha^2 * beta)]^(-beta) |
Comment on lines
+144
to
+148
| def _sources_match(self, source_ras: npt.NDArray[Any], source_decs: npt.NDArray[Any]) -> bool: | ||
| if self.source_ras is None: | ||
| return False | ||
| if self.sources_decs is None: | ||
| return False |
Comment on lines
170
to
173
| if self._events_match(events): | ||
| return | ||
| if self._sources_match(source_ras, source_decs): | ||
| return |
Comment on lines
+202
to
+205
| if len(source_ras) == 1 and cutoff < np.pi: | ||
| src_ra = float(source_ras[0]) | ||
| src_dec = float(source_decs[0]) | ||
| ra_span = min(cutoff / max(abs(np.cos(src_dec)), np.sin(cutoff)), np.pi) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Use the CDF for fitting instead of the PDF for more stability. Use scipy's minimize instead with a numerically calculated gradient and skip the grid search.
The CDF is more expensive to calculate, so this is noticeably slower... but in a very small set of trials, I'm seeing much better ns behavior.I've updated the CDF calculation to use an analytic form. This actually is faster than the interpolation that was in InterpolatedKingPDF by ~10x, making that class obsolete. Because of this, I've removed the InterpolatedKingPDF entirely, updated the tests, and updated the docs.
In order to simplify and consolidate the code, the KingSpatialLikelihood class no longer accepts a skymap. A separate KingTemplateLikelihood will be built later to replace this incomplete functionality.