fix: make optimizer post-processing robust to correlations without uncertainties#2708
Open
henryiii wants to merge 1 commit into
Open
fix: make optimizer post-processing robust to correlations without uncertainties#2708henryiii wants to merge 1 commit into
henryiii wants to merge 1 commit into
Conversation
…certainties Hoist `num_fixed_pars` in `_internal_postprocess` so the correlations branch no longer raises an UnboundLocalError when a fit result carries `corr` without `unc`. Also make a defensive copy of `par_names` in `minimize` so duck-typed configs returning a stored list are not corrupted by the in-place mutation. Assisted-by: ClaudeCode:claude-opus-4.8
📝 WalkthroughWalkthroughThis PR fixes two defensive issues in ChangesOptimizer defensive fixes and postprocessing repair
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/test_optim.py`:
- Around line 604-623: Update
test_postprocess_correlations_without_uncertainties to also cover num_fixed_pars
> 0 by replacing the passthrough stitch_pars with one that actually stitches in
fixed parameters (use _make_stitch_pars and _TensorViewer or manually create a
stitch_pars that inserts fixed values) so that
OptimizerMixin._internal_postprocess is exercised when correlation stitching
must add rows/columns; call optimizer._internal_postprocess with the new
stitch_pars and add assertions that result.corr has the expected larger shape
(and that the fixed-parameter rows/columns are zeros or the expected values) to
validate the stitching logic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3200d03d-d191-4165-8e18-4e7a73660f05
📒 Files selected for processing (2)
src/pyhf/optimize/mixins.pytests/test_optim.py
Comment on lines
+604
to
+623
| def test_postprocess_correlations_without_uncertainties(): | ||
| # Regression test: _internal_postprocess must not raise when a fit result | ||
| # carries correlations but no uncertainties (num_fixed_pars was previously | ||
| # only defined inside the uncertainties branch). | ||
| pyhf.set_backend("numpy") | ||
|
|
||
| fitresult = OptimizeResult( | ||
| x=np.asarray([1.0, 2.0]), | ||
| fun=0.0, | ||
| corr=np.asarray([[1.0, 0.5], [0.5, 1.0]]), | ||
| ) | ||
|
|
||
| def stitch_pars(pars, stitch_with=None): # noqa: ARG001 | ||
| return pars | ||
|
|
||
| optimizer = OptimizerMixin() | ||
| result = optimizer._internal_postprocess(fitresult, stitch_pars) | ||
|
|
||
| assert result.unc is None | ||
| assert np.allclose(pyhf.tensorlib.tolist(result.corr), [[1.0, 0.5], [0.5, 1.0]]) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Consider testing with num_fixed_pars > 0 for more thorough coverage.
The regression test correctly validates that _internal_postprocess no longer raises UnboundLocalError when correlations exist without uncertainties. However, the passthrough stitch_pars means num_fixed_pars = 0, so the correlation stitching logic (lines 105-115 in mixins.py) runs but doesn't add rows/columns.
A more comprehensive test would use a stitch_pars that actually adds fixed parameters, ensuring the correlation stitching logic works correctly with the hoisted num_fixed_pars computation.
📝 Example enhancement
def test_postprocess_correlations_without_uncertainties():
# ... existing setup ...
# Test with fixed parameters
from pyhf.optimize.common import _make_stitch_pars
from pyhf.tensor.common import _TensorViewer
fixed_idx = [0]
variable_idx = [1, 2]
tv = _TensorViewer([fixed_idx, variable_idx])
stitch_pars = _make_stitch_pars(tv, [1.5])
fitresult = OptimizeResult(
x=np.asarray([2.0, 3.0]), # variable params only
fun=0.0,
corr=np.asarray([[1.0, 0.5], [0.5, 1.0]]), # 2x2 for variable params
)
result = optimizer._internal_postprocess(fitresult, stitch_pars)
# Should stitch in zeros for fixed param: result shape (3, 3)
assert result.corr.shape == (3, 3)🧰 Tools
🪛 Ruff (0.15.15)
[warning] 616-616: Missing return type annotation for private function stitch_pars
(ANN202)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_optim.py` around lines 604 - 623, Update
test_postprocess_correlations_without_uncertainties to also cover num_fixed_pars
> 0 by replacing the passthrough stitch_pars with one that actually stitches in
fixed parameters (use _make_stitch_pars and _TensorViewer or manually create a
stitch_pars that inserts fixed values) so that
OptimizerMixin._internal_postprocess is exercised when correlation stitching
must add rows/columns; call optimizer._internal_postprocess with the new
stitch_pars and add assertions that result.corr has the expected larger shape
(and that the fixed-parameter rows/columns are zeros or the expected values) to
validate the stitching logic.
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
1 participant
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.
🤖 AI text below 🤖
Part of the code review in #2706.
Summary
Two correctness fixes in
src/pyhf/optimize/mixins.py:_internal_postprocess:num_fixed_parswas only defined inside theif uncertainties is not None:branch but used unconditionally in theif correlations is not None:branch. A fit result carryingcorrwithoutunctherefore raised anUnboundLocalError. The assignment is now hoisted to right afterfitted_parsis computed so both branches can use it.minimize:par_names = pdf.config.par_nameswas mutated in place (par_names[index] = None). pyhf's own property returns a fresh list, but duck-typed configs returning a stored list would be corrupted. A defensive copy (list(...)) is now made whenpar_namesis notNone.Test plan
test_postprocess_correlations_without_uncertaintiesintests/test_optim.py, which exercises_internal_postprocesswith anOptimizeResultthat hascorrbut nounc. It fails withUnboundLocalErroronmainand passes with this change.pytest tests/test_optim.py -q: 73 passed, 5 skipped.prek -a(ruff lint/format): clean.🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Bug Fixes
Tests