Re-arm regularizers when the parameter layout changes (saturated GoF was unwalled) - #149
Merged
Merged
Conversation
A regularizer that resolves a parameter position is only correct for the
layout it resolved against, and the layout is not stable within a fit
session. The saturated goodness-of-fit path swaps the model for
CompositeParamModel([original, SaturatedProjectModel]), whose
[poi(m1), poi(m2), ..., pou(m1), pou(m2), ...] ordering inserts one POI per
projected bin ahead of the original model's block. In a 2D ptll x yll
SCETlib fit that displaced all 7 lambdas by 39 positions.
Nothing told the regularizers. rabbit_fit.py restores `regularizers` after
the composite re-init but never re-arms them, and the arming loop lives in
Fitter.defaultassign(), which that path does not call -- it calls
xdefaultassign(), so the saturated fit starts from the parameter defaults.
For WRemnants' NPDampingWall, which caches positions at construction, that
meant reading saturated_ch0_ptll0..6 -- per-bin factors near 1.0, far above
its 0.005 margin -- as the 7 lambdas. Every wall condition was trivially
satisfied and the penalty was identically zero, so the saturated REFERENCE
ran unwalled while the NOMINAL fit it is compared against was walled. The
reference then parked at lambda2_nu = -0.231, which costs 1225 nll units
once the wall is applied.
Measured on 2D real-data fits, the goodness-of-fit numbers move:
MSHT20 prior-free chi2 47.58 -> 62.11 p 16.28% -> 1.07%
MSHT20 lattice cov chi2 33.44 -> 65.56 p 72.10% -> 0.49%
CT18Z prior-free chi2 57.19 -> 56.29 p 3.01% -> 3.60%
A guard for this already existed (a width check in set_expectations whose
comment names the saturated path) but sits on a code path that fit does not
take, so it never re-armed and never fired.
* Regularizer.set_expectations gains `parms`, the names of the current
vector, defaulted to None so existing implementations keep working.
* Regularizer.resolve_indices(parms, names) resolves by name and raises on a
missing name.
* Fitter.arm_regularizers() passes self.parms; defaultassign() delegates to
it; rabbit_fit.py calls it after the composite re-init.
* init_fit_parms marks regularizers stale and _compute_nll_components
refuses to evaluate a penalty while they are, so a future path that
changes the layout and forgets to re-arm crashes instead of silently
applying each regularizer at its old positions.
Also persists the saturated fit under
results["mappings"][<mapping>]["saturated_fit"] (parms / nllvalreduced /
edmval / cov / epoch_loss / minimizer_status), using the same key names the
primary fit uses at top level. Previously that fit's whole state was
discarded and only its chi2 survived, so the reference's parameters, the
per-bin residuals, its convergence and nll_sat were all unrecoverable from
the output. That is what made this bug visible.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CnJ9YKK8c1q1sCDouM6y1c
lucalavezzo
force-pushed
the
saturated-fit-diagnostics
branch
from
August 10, 2026 22:36
e122046 to
c157d6e
Compare
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.
Separate from #147 — that one is about the external-term constant, this one is about regularizers reading the wrong parameters.
The bug
A regularizer that resolves a parameter position is only correct for the layout it resolved against — and the layout is not stable within a fit session. The saturated goodness-of-fit path swaps the model for
CompositeParamModel([original, SaturatedProjectModel]), whose[poi(m1), poi(m2), ..., pou(m1), pou(m2), ...]ordering inserts one POI per projected bin ahead of the original model's block. In a 2D ptll × yll SCETlib fit that displaces all 7 λ by 39 positions.Nothing tells the regularizers.
rabbit_fit.pyrestoresregularizersafter the composite re-init but never re-arms them, and the arming loop lives inFitter.defaultassign()— which that path doesn't call. It callsxdefaultassign(), deliberately, so the saturated fit starts from the parameter defaults.For WRemnants'
NPDampingWall, which caches positions at construction, the effect was:Per-bin factors near 1.0, far above the wall's 0.005 margin — so every condition was trivially satisfied and the penalty was identically zero. The saturated reference ran unwalled while the nominal fit it is compared against was walled: a likelihood ratio whose two halves had different objectives. The reference duly parked at
lambda2_nu = -0.231, a point that costs 1225 nll units once the wall is applied.Impact on results
Measured on 2D real-data fits — same card, same minimiser, same start, only the wall now readable:
Directly verified: the broken reference's postfit
lambda2_nu = -0.23081(wall would charge 1224.8); the fixed one lands at+0.00488, resting on the 0.005 margin, with the wall charging 0.000.There was already a guard
set_expectationshas a width check whose comment names this exact scenario ("a wrapping/composite param model (e.g. the saturated goodness-of-fit path) reorders/resizes the block in a way this wall's flat indexing cannot follow"). It never fired, because it sits on a code path the saturated fit doesn't take.Related:
7bd0d81fixed the x0 permutation for this same layout; the line directly below it re-pointsregularizerswithout rebinding them.Changes
Regularizer.set_expectationsgainsparms(names of the current vector), defaulted toNoneso existing implementations keep working. Docstring now states the layout can be reordered and resized mid-session and that positions must not be cached — it previously said "called once per fit configuration", which reads as an invitation to cache.Regularizer.resolve_indices(parms, names)resolves by name and raises on a missing name, so the easy path is the correct one.Fitter.arm_regularizers()passesself.parms;defaultassign()delegates to it;rabbit_fit.pycalls it after the composite re-init.init_fit_parmsmarks regularizers stale;_compute_nll_componentsrefuses to evaluate a penalty while they are. A future path that changes the layout and forgets to re-arm crashes instead of silently applying each regularizer to whatever sits at its old positions.Also included: persisting the saturated fit
Under
results["mappings"][<mapping>]["saturated_fit"], using the same key names the primary fit uses at top level (parms/nllvalreduced/edmval/cov/epoch_loss/minimizer_status), so a reader can treat the sub-group exactly like the main results dict.Previously that fit's entire state was discarded and only its chi² survived, which made ordinary questions unanswerable from the files: where the reference put its parameters (the
parmsbeside the chi² are the nominal ones echoed back by the outer--noFitpass); which bins the model misses (the saturated POIs are one per projected bin, so their postfit values are a per-bin residual map); whether the reference converged (this step runs--noHessian, so noedmval, and scipy's outcome waslogger.debug-only); and whatnll_satis (recoverable only by inverting the chi² definition).A few tens of kB against a ~100 MB cov. It's in this PR because it is what made the bug visible.
Notes for reviewers
svd.pyneeds no logic change (it holds no per-parameter positions); its signature is widened for interface compatibility.tests/test_regularizer_layout.pycovers name resolution across two layouts, refusal to guess on a missing name or absentparms, and the backward-compatible signature.black/isort --profile black/flake8with the CI flags, run in the CI container image, clean repo-wide.🤖 Generated with Claude Code
https://claude.ai/code/session_01CnJ9YKK8c1q1sCDouM6y1c