fix: masked BinnedNLL gradient by henryiii · Pull Request #1137 · scikit-hep/iminuit

henryiii · 2026-06-11T14:20:39Z

🤖 AI text below 🤖

The masked renormalization correction in BinnedNLL._grad summed the model gradient over both axes, which gave one scalar for all parameters. It must give one correction per parameter. Any masked BinnedNLL fit that used the analytic gradient got silently wrong results.

The mask selects along the first axis only, so for a multi-dimensional histogram the correction must also sum over the remaining bin axes, then reshape to broadcast over the prediction.

The existing test_BinnedNLL_mask uses a one-parameter 1D model, for which the sums agree, so the bug was not seen. The new tests compare against a numerical gradient with two parameters in 1D and with a masked 2D histogram.

Reported in #1132.

henryiii · 2026-06-15T12:46:38Z

🤖 AI text below 🤖

Rebased onto develop. While verifying CI I found the coverage job still failing on test_CostSum_visualize — but it's a separate, pre-existing break on develop (not fixed by #1140): matplotlib ≥ 3.11 now raises ValueError: Figure N already exists when plt.subplots(num=fig.number) reuses the current figure. Since this PR already touches that exact line for the squeeze=False fix, I folded in clear=True here. Verified locally (built the wheel, all test_cost.py::*visualize* pass; clear=True preserves the set_figwidth call and yields the correct ax shape).

henryiii · 2026-06-15T14:09:01Z

@HDembinski This is needed to get the rest of the PRs to go green (the clear=True fix above), so reviewing this next would be good.

(Note: "draft" means I have not reviewed it yet, I'm taking these out of draft when I review; you can review at any time)

HDembinski · 2026-06-16T04:25:39Z

Since this is AI generated, these should be individual focused PRs, not this hard to review collection of fixes.

henryiii · 2026-06-16T04:32:25Z

Let me pull out the one that helps with CI, then I'll split the rest up.

henryiii · 2026-06-16T04:33:44Z

(Fully doing one per fix would have hit the CI pretty hard, but I can break them up one at a time if that helps)

henryiii · 2026-06-16T04:45:40Z

(Need to figure out how to stop Claude from commenting every rebase here, normally it doesn't do that)

…1148) 🤖 _AI text below_ 🤖 Split out of #1137. A model that returned a non-float sequence took an early return before the shape check, so a wrong shape passed silently. The value is now converted to float and falls through to the shape check.

🤖 _AI text below_ 🤖 Split out of #1137. `scaled_pdf` must scale by the number of data points. For multivariate data of shape `(D, N)`, `np.prod(data.shape)` gives `D * N`, so the number density was too large by the factor `D`. `data.shape[-1]` is correct for 1D and multivariate data. Reported in #1132.

🤖 _AI text below_ 🤖 Split out of #1137. `plt.subplots(1, 1)` returns a bare Axes, not an array, so indexing it raised a `TypeError` when the sum contained exactly one component with a `visualize` method. The fix passes `squeeze=False`. Also removes the `**kwargs` paragraph from the docstring. The method does not accept `**kwargs`. Reported in #1132. Co-authored-by: Eduardo Rodrigues <eduardo.rodrigues@cern.ch>

🤖 _AI text below_ 🤖 Two bugs in the pull computation, split out of #1137. - An index-array mask was negated with `~`, which is bitwise NOT for integers, so the wrong bins became NaN. The new helper `_exclusion_from_mask` handles boolean masks and index masks. - The user mask replaced the `err == 0` mask instead of combining with it, so bins with zero error gave ±inf pulls instead of NaN. `LeastSquares.pulls` now also gives NaN for zero-error points when no mask is set (before: ±inf), consistent with `BinnedCost`. The `pulls` docstring now states this. Reported in #1132.

ikrommyd

Please consider the comment for the 2D case. It's correct for 1D already. I would have thought of checking this myself tbh so fable is probably right here. If it's right, you can add the change with a 2 line comment explaining it + a test for the 2D case.

ikrommyd · 2026-09-02T12:24:52Z

        if ma is not None:
            psum = np.sum(p[ma])
-            pg = pg / psum - p * np.sum(pg[:, ma]) / psum**2
+            pg = pg / psum - p * np.sum(pg[:, ma], axis=1)[:, np.newaxis] / psum**2


This is correct for 1D but fable informs me that this is still wrong for 2D masked gradients. It suggested this change but I haven't out the bath myself.

Suggested change

pg = pg / psum - p * np.sum(pg[:, ma], axis=1)[:, np.newaxis] / psum**2

corr = np.sum(pg[:, ma].reshape(pg.shape[0], -1), axis=1)

pg = pg / psum - p * corr.reshape((-1,) + (1,) * p.ndim) / psum**2

🤖 AI text below 🤖

Confirmed and applied. BinnedNLL.mask selects along the first axis only, so for a 2D histogram pg[:, ma] keeps the trailing bin axis and sum(axis=1) left one correction per column instead of one per parameter. It broadcast without an error, so the result was silently wrong.

Added test_BinnedNLL_2D_mask_grad, which compares a masked 2D gradient to the numerical gradient. It fails on the old code (all 4 elements mismatch) and passes now.

The renormalization correction summed the model gradient over both axes, which gave one scalar for all parameters. It must be summed over the bin axis only, so that each parameter gets its own correction. The existing mask test uses a one-parameter model, for which the two sums agree, so the bug was not seen. The new test compares against a numerical gradient with two parameters. Reported in #1132. Assisted-by: ClaudeCode:claude-opus-5

The mask selects along the first axis only, so the renormalization correction must also sum over the remaining bin axes and reshape to broadcast over the prediction. Assisted-by: ClaudeCode:claude-opus-5

ikrommyd

Should be good now. Thanks!

henryiii mentioned this pull request Jun 11, 2026

Claude Fable review for iminut #1132

Open

henryiii force-pushed the fix/cost-bugs-1132 branch 2 times, most recently from c09efff to 156f4bb Compare June 15, 2026 12:46

henryiii marked this pull request as ready for review June 15, 2026 14:22

henryiii mentioned this pull request Jun 16, 2026

fix: matplotlib >= 3.11 figure reuse in CostSum.visualize (repairs coverage CI) #1141

Merged

henryiii force-pushed the fix/cost-bugs-1132 branch from 156f4bb to 774e072 Compare June 16, 2026 04:39

This comment was marked as low quality.

Sign in to view

henryiii marked this pull request as draft June 16, 2026 04:46

henryiii force-pushed the fix/cost-bugs-1132 branch 2 times, most recently from a69c1f6 to ae5b41a Compare July 27, 2026 14:28

henryiii changed the title ~~fix: cost function bugs (masked BinnedNLL gradient, pulls masks, visualize)~~ fix: gradient of masked BinnedNLL with two or more parameters Jul 27, 2026

henryiii changed the title ~~fix: gradient of masked BinnedNLL with two or more parameters~~ fix: masked BinnedNLL gradient Jul 27, 2026

henryiii marked this pull request as ready for review September 1, 2026 18:04

ikrommyd requested changes Sep 2, 2026

View reviewed changes

fix: masked BinnedNLL gradient for multi-dimensional histograms

0ebf54e

The mask selects along the first axis only, so the renormalization correction must also sum over the remaining bin axes and reshape to broadcast over the prediction. Assisted-by: ClaudeCode:claude-opus-5

henryiii force-pushed the fix/cost-bugs-1132 branch from ae5b41a to 0ebf54e Compare September 2, 2026 14:49

ikrommyd approved these changes Sep 2, 2026

View reviewed changes

henryiii merged commit 78ffc81 into develop Sep 2, 2026
9 checks passed

henryiii deleted the fix/cost-bugs-1132 branch September 2, 2026 15:16

Navigation Menu

Uh oh!

fix: masked BinnedNLL gradient - #1137

fix: masked BinnedNLL gradient#1137
henryiii merged 2 commits into
developfrom
fix/cost-bugs-1132

henryiii commented Jun 11, 2026 •

edited

Loading

Uh oh!

henryiii commented Jun 15, 2026

Uh oh!

henryiii commented Jun 15, 2026

Uh oh!

HDembinski commented Jun 16, 2026

Uh oh!

henryiii commented Jun 16, 2026

Uh oh!

henryiii commented Jun 16, 2026

Uh oh!

This comment was marked as low quality.

henryiii commented Jun 16, 2026

Uh oh!

ikrommyd left a comment •

edited

Loading

Uh oh!

ikrommyd Sep 2, 2026

Uh oh!

henryiii Sep 2, 2026

Uh oh!

ikrommyd left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	pg = pg / psum - p * np.sum(pg[:, ma], axis=1)[:, np.newaxis] / psum**2
	corr = np.sum(pg[:, ma].reshape(pg.shape[0], -1), axis=1)
	pg = pg / psum - p * corr.reshape((-1,) + (1,) * p.ndim) / psum**2

Uh oh!

Conversation

henryiii commented Jun 11, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

henryiii commented Jun 15, 2026

Uh oh!

henryiii commented Jun 15, 2026

Uh oh!

HDembinski commented Jun 16, 2026

Uh oh!

henryiii commented Jun 16, 2026

Uh oh!

henryiii commented Jun 16, 2026

Uh oh!

This comment was marked as low quality.

henryiii commented Jun 16, 2026

Uh oh!

ikrommyd left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ikrommyd Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

henryiii Sep 2, 2026

Choose a reason for hiding this comment

Uh oh!

ikrommyd left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

henryiii commented Jun 11, 2026 •

edited

Loading

ikrommyd left a comment •

edited

Loading