CompositeParamModel: preserve the [POIs | POUs] parameter layout#143
Merged
Conversation
CompositeParamModel summed npoi but concatenated params in model order, so a POU-carrying model placed before a POI-carrying one leaked its POUs into the composite's POI slice. Every npoi-sliced consumer then misbehaved: get_poi() squared the POUs (allowNegativeParam=False default) and, on real data, blinded them as signal-strength modifiers -- visible with --computeSaturatedProjectionTests + a custom npou>0 param model, where the saturated refit ran with squared+blinded theory parameters (frozen ones unable to compensate), invalidating the saturated p-value. Fix: assemble the composite per-block, [poi(m1), poi(m2), ... | pou(m1), pou(m2), ...], and reassemble each submodel's native [poi | pou] vector in compute(). Legacy-valid orderings produce identical layouts. Additionally: - allowNegativeParam is derived from the POI-carrying submodels only (mixed flags raise: the fitter applies a single squaring transform to the POI block); load_models' any() over all models had the same trap. - param_impact_groups are merged (name-based, permutation-safe), and optional submodel prior_sigmas/prior_means propagate through the same permutation (NaN = no prior) for forward-compatibility with priors. - is_linear no longer claims linearity for products of >1 parameter-dependent factor or sqrt-stored POIs. Tested in tests/test_composite_param_model.py (layout, compute slicing vs manual evaluation, gradient flow, legacy-ordering invariance, flag derivation/conflicts, prior+group propagation). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Dropped per request; it was not wired into the CI unit-test matrix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
lucalavezzo
added a commit
to lucalavezzo/rabbit
that referenced
this pull request
Jun 23, 2026
Integrates the merged sibling PRs into the priors/unification branch: WMass#140 fit-ops (minimizer flags, externalPostfit), WMass#142 scalePoiScan, WMass#143 CompositeParamModel layout fix, WMass#144 blinding groups. Conflicts (both trivial): - fitter.py: blinding_group=options.blindingGroup (main's direct form; the SimpleNamespace test fixtures already carry blindingGroup=[]). - helpers.py: kept main's cleaned-up version (dropped the explanatory comment removed on main). Also dropped tests/test_composite_param_model.py, which the merge re-introduced from this branch but was deliberately removed on main. Validated: lint clean; pytest (15) + the five CI script-tests pass; the ParamModel-prior closure is unchanged (gaussian sig_prior 1.105769e-02, asym (-1.097e-2,+1.114e-2)); parsing.py is identical to main so all of WMass#140/WMass#142/WMass#144's CLAs are present. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
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
2 participants
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.
Split out of #139 as a standalone bugfix (no dependency on the priors work).
Problem
CompositeParamModelsummednpoibut concatenatedparamsin model order, so a POU-carrying model placed before a POI-carrying one leaked its POUs into the composite's POI slice. Everynpoi-sliced consumer then misbehaved:get_poi()squared the POUs (allowNegativeParam=Falsedefault) and, on real data, blinded them as signal-strength modifiers — visible with--computeSaturatedProjectionTests+ a customnpou>0param model, where the saturated refit ran with squared+blinded theory parameters (frozen ones unable to compensate), invalidating the saturated p-value.Fix
Assemble the composite per-block,
[poi(m1), poi(m2), … | pou(m1), pou(m2), …], and reassemble each submodel's native[poi | pou]vector incompute(). Legacy-valid orderings produce identical layouts. Also:allowNegativeParamis derived from the POI-carrying submodels only (mixed flags raise — the fitter applies a single squaring transform to the POI block);load_models'any()over all models had the same trap.param_impact_groupsare merged (name-based, permutation-safe); optional submodelprior_sigmas/prior_meanspropagate through the same permutation for forward-compat.is_linearno longer claims linearity for a product of >1 parameter-dependent factor or sqrt-stored POIs.Tests
New
tests/test_composite_param_model.py: layout,computeslicing vs manual evaluation, gradient flow, legacy-ordering invariance, flag derivation/conflicts, prior+group propagation. (6 tests, pass.)🤖 Generated with Claude Code