pyBumpHunter review with Claude Opus 4.8

[henryiii]

Do you want me to generate fixes for any or all of these?

🤖 AI text below 🤖

I now have a thorough picture. The test suite passes (5 tests, with matplotlib/uproot installed), and I've confirmed several bugs by running code. Here's the review.

Bugs

1. BumpHunter deprecated alias passes args to the wrong parameters — confirmed broken (__init__.py:80-101)
The super().__init__(...) call is positional and was written against an older signature. Since npe_inject and sideband_width were later inserted into BumpHunter1D.__init__, every argument from position 17 on is shifted. I verified: BumpHunter(seed=42) lands 42 in npe_inject, leaves seed=None, and sets use_sideband=None and sideband_width=<Nworker default> instead of their defaults. Fix: pass everything by keyword.

2. load_state overwrites rang with weights and never restores weights (bumphunter_1dim.py:806-809, same in bumphunter_2dim.py:919-921)

if "weights" in state:
    self.rang = state["weights"]   # should be self.weights
else:
    self.rang = None               # clobbers rang loaded just above

Confirmed: after a save/load round-trip, rang becomes the weights value and weights is never set from state.

3. load_state restores flip_sig into the wrong attribute (bumphunter_1dim.py:876-879, bumphunter_2dim.py:989-992)
save_state writes state["sig_flip"] = self.flip_sig, but load_state does self.sig_flip = state["sig_flip"] — setting a dead sig_flip attribute while the real flip_sig is never loaded. Also sideband_width is never saved or loaded at all, and load_state defaults width_min to 2 while the constructor default is 1.

4. Thread-pool exceptions are silently swallowed (bumphunter_1dim.py:1110-1149, 1297-1302, 1389-1396; 2D 1419, 1507)
exe.submit(...) results are never collected, so any exception raised inside _scan_hist in a worker thread is lost and the result arrays silently keep np.empty(...) garbage. Collect the futures and call .result() (or use exe.map) so failures surface. This matters more now that CI runs free-threaded 3.14 where these truly run in parallel.

5. _scan_hist crashes on an all-zero reference (bumphunter_1dim.py:363-364)
non0 = [i for i in range(hist.size) if ref[i] > 0] then min(non0) raises ValueError: min() arg is an empty sequence if a pseudo-experiment/reference has no positive bin. Worth a guard.

6. do_pseudo=False re-run hits an array-truth ambiguity (bumphunter_1dim.py:1082, 1094)
if self.min_Pval_ar == []: works right after reset() (it's a list), but after a prior scan min_Pval_ar is an ndarray and ndarray == [] does not give a clean bool. Use len(self.min_Pval_ar) == 0 or a sentinel.

7. 2D signal_inject appears to be dead/broken (bumphunter_2dim.py:~1360-1510)
CLAUDE.md states 2D doesn't support signal injection, yet a signal_inject exists that calls self._scan_hist with 1D [:, th] indexing — inconsistent with the 2D scan signature. It's untested. Either remove it or make it raise NotImplementedError explicitly.

8. util.deprecated_arg misuses its data structure (util.py:7,33)
warned_args = defaultdict(dict) but line 33 assigns warned_args[func] = oldarg (a string), and the guard oldarg not in warned_args[func] then does a substring check. It happens to work for one deprecated arg per function but breaks the moment a function deprecates two args. Use a set keyed by (func, oldarg).

Performance

9. The window-sum loop is the main hotspot — replace with prefix sums (bumphunter_1dim.py:411-412, the # FIXME ... Without loop ?? comment, and the 2D analog)

Nref = np.array([ref[p:p+w].sum() for p in pos], dtype=float)
Nhist = np.array([hist[p:p+w].sum() for p in pos])

This is O(npe × n_widths × n_positions × w). Precompute cumref = np.concatenate([[0], ref.cumsum()]) once per histogram, then each window sum is cumref[pos+w] - cumref[pos] — vectorized and O(1) per window. This is the single biggest speedup available and removes the per-window Python loop entirely.

10. np.tile(bkg_hist, (npe,1)).transpose() is wasteful (bumphunter_1dim.py:1062-1065 etc.)
np.random.poisson broadcasts; you can pass lam=bkg_hist[:, None] (or just rely on size) instead of materializing a tiled copy.

11. [i for i in range(ref.size) if ref[i] > 0] (bumphunter_1dim.py:363, 509, 1562, 1565) — replace with np.nonzero(ref)[0]; faster and clearer.

Simplifications

12. load_state is ~120 lines of repetitive if "x" in state blocks — collapse to a defaults dict + loop, e.g. self.x = state.get("x", default). This would also have prevented bugs #2/#3.

13. Legacy-arg handling is duplicated (bumphunter_1dim.py:287-292) — the @deprecated_arg decorators already exist, but the constructor also manually does if useSideBand is not None: .... The manual block is what actually does the work; the decorators only warn. Fine to keep, but it's worth a comment, or fold the remapping into one place.

14. Trailing bare return statements throughout (reset, bump_scan, plot methods) are noise.

Modernizations

15. Use numpy.random.Generator instead of the legacy global RNG (np.random.seed / np.random.poisson, many sites). rng = np.random.default_rng(self.seed) is reproducible, thread-safer, and the recommended API. signal_inject even calls np.random.seed(self.seed) twice (1239 and 1272).

16. Make matplotlib a lazy/optional import (bumphunter_1dim.py:7-9, 2D top). It's imported at module load, so the core algorithm can't be used without matplotlib installed (I hit ModuleNotFoundError on a bare env even though plotting wasn't used). Import it inside the plot methods and move it to an optional extra.

17. Typos in the public docstring/__init__.py — "BumpHenter", "BumpHnuter", "valid of BumpHunter2D" (should be "valid for"), "Ruturns", "durring", "possition". The __init__.py module docstring also advertises only the deprecated CamelCase API (BumpScan, GetTomography, PlinInject).

18. print(...) for progress — the algorithm prints unconditionally to stdout. A verbose flag or logging would be more library-friendly (out of scope for a quick fix, but worth noting).

[eduardo-rodrigues]

Hi @henryiii, you are fast! I was delaying to ping you for this kind of stuff as @KaranSinghDev is a young contributor that got in touch about Scikit-HEP and has been many very welcome improvements and fixes, see his grand task #27 and related PRs. He is finalising. Best to liaise as I'm sure Claude caught a few more things.

[henryiii]

Yes, I triggered this because I'm watching that issue and it was nearly done. I don't think there's any overlap, this is all looking for bugs and such, not modernizing packaging. (For packaging, I'd also recommend going setuptools -> hatchling or uv_build or flit-core). It didn't noticed the old references to 3.8, for example (IIUC those are still there).

[eduardo-rodrigues]

OK, cool.

Yep, I also realised the mention of 3.8 earlier this morning and mentioned it in #27.

@KaranSinghDev will have to wait until next week to resume work. That's no problem.

[henryiii]

@KaranSinghDev please let me know, when you are around, if and/or when you'd like me to generate fixes for some or all of these. You can see others I've done at https://iscinumpy.dev/post/claude-code-reviews/.

[KaranSinghDev]

@henryiii Thank you for this, there many findings I see which I think we should act on, I have to actually give some tests for an application this week and I think I can work on this after June 22 and if you would like to start on some of the fixes, please do not let my pause interrupt it.

[henryiii]

I can trigger them and let you review them at your leisure. :)

[henryiii]

🤖 AI text below 🤖

Opened PRs for the actionable findings from the review above. cc @KaranSinghDev

PRs

• fix: correct parameter and state save/load handling #38 — parameter & state-handling bugs: the deprecated BumpHunter alias was passing args to the wrong parameters; load_state was writing weights into rang and never restoring weights/flip_sig/sideband_width; and deprecated_arg's warning bookkeeping was misusing its data structure. (covers review items 1, 2, 3, 8)
• fix: surface scan worker errors and fix array truthiness checks #36 — scan robustness: thread-pool worker exceptions were silently swallowed (futures never collected); also fixed an ndarray == [] truthiness check on scan re-runs. (items 4, 6)
• perf: use prefix sums for window counts in scan #37 — performance: window counts now use prefix sums instead of re-summing overlapping slices, plus a small np.nonzero/empty-reference guard and dropping a redundant np.tile. Results are byte-identical and the validated tests still pass. 1D only; 2D deferred (needs a 2D summed-area table). (items 9, 10, 11, 5)
• chore: fix docstring typos #39 — cleanup: matplotlib is now imported lazily inside the plot methods, so the core algorithm imports without matplotlib installed; plus docstring/comment typo fixes. (items 16, 17)

Left unfixed (need a decision or deferred on purpose)

• 2D signal_inject (item 7): it exists but calls the 1D-style _scan_hist and is untested, despite the docs saying 2D doesn't support injection. Needs a call: remove it, or make it raise NotImplementedError? I left it untouched.
• RNG modernization (item 15): switching from the legacy global np.random.seed/poisson to np.random.default_rng would change the random stream and break the validated exact test values (e.g. global p-value 0.01770). This needs the oracle constants to be regenerated/relaxed first, so I deferred it.
• 2D prefix-sum perf: deferred as a follow-up (see perf: use prefix sums for window counts in scan #37).
• Progress print()s (item 18): the algorithm prints progress to stdout unconditionally. Moving to logging or a verbose flag is an API choice, so left as-is.
• Simplifications (items 12–14): the repetitive load_state if/else chain, the duplicated deprecated-arg remapping, and trailing bare returns are cosmetic — not addressed here.

[eduardo-rodrigues]

Hello @KaranSinghDev, related to our chat on getting the package on to maintenance mode - there are a few things "left unfixed" from the list above, which we may want to adress. Then we are good and will be able to close here.