pyhf review from Claude Fable

[henryiii]

🤖 AI text below 🤖

Review complete. I fanned out subagents across the five major subsystems, then personally verified every load-bearing claim against the source — discarding several false positives along the way (e.g. "_TensorViewer caches the backend" — no, its pyhf.default_backend use is intentional setup-time index math with a proper tensorlib_changed subscription; and "ToyCalculator re-fits redundantly" — the pre-fits generate toys, the per-toy fits compute the test statistic; both are required). Everything below is verified unless marked otherwise. Nothing was modified.

Bugs

Likely worth fixing

1. src/pyhf/optimize/mixins.py:87,105 — latent NameError in _internal_postprocess. num_fixed_pars is defined only inside if uncertainties is not None: but used unconditionally in the if correlations is not None: block. Today minuit sets both unc and corr so it never fires, but any result with correlations and no uncertainties crashes. Hoist the assignment above both blocks.

2. src/pyhf/events.py:51-75 — dead plain-function callbacks are called as None(...). For bound methods, __call__ checks the __self__ weakref and _flush() prunes dead ones. But plain functions are stored as (weakref.ref(callback), None): _flush keeps every arg is None entry forever, and __call__ line 75 invokes func()(*args) without checking, so a garbage-collected function (e.g. a locally-defined subscriber, common in tests) raises TypeError: 'NoneType' object is not callable on the next set_backend.

3. src/pyhf/parameters/utils.py:46-62 — two defects in reduce_paramsets_requirements. (a) After combined_paramset[k].pop(), the continue for "undefined" leaves an empty set as the value for every attribute the modifier doesn't support; downstream only survives because paramset constructors test truthiness (if sigmas:). (b) The elif ordering compares user lists against the literal string "undefined", so configuring an unsupported attribute (e.g. sigmas on a normsys parameter) with anything other than exactly 9 elements yields "expected 9" instead of the intended "does not use the sigmas attribute" error. Check default_v == "undefined" first and del the key on skip.

4. src/pyhf/constraints.py:138 (and the poisson twin ~:259) — broken no-constraint fallback. tensorlib.astensor(0.0)[0] indexes a 0-d array and raises IndexError on both backends. Unreached via Model.logpdf (which goes through make_pdf), but anyone calling *_constraint_combined.logpdf directly with batch_size=None and no constraints crashes. Drop the [0].

5. src/pyhf/readxml.py:218 — KeyError on missing Activate. modtag.attrib["Activate"] hard-indexes while every sibling attribute uses .get(...) with a default; a <StatError> without Activate kills the whole XML import with a raw KeyError. Use .get("Activate", "False").

6. CLI file-handle leaks — src/pyhf/cli/infer.py:103,199, src/pyhf/cli/rootio.py:97. json.loads(click.open_file(pfile, ...).read()) never closes the handle, while all ~15 neighboring sites correctly use with. Harmless under CPython refcounting, but inconsistent and warns under -X dev/PyPy.

7. src/pyhf/readxml.py:535-537 — clear_filecache() drops uproot files without closing them, and the module-global __FILECACHE__ holds every opened ROOT file open for the life of the process. Close handles before clearing.

8. src/pyhf/patchset.py:179 — vestigial sentinel keys. _patches_by_key = {"name": {}, "values": {}} but patches are stored flat in the same dict. Lookups work, but a patch literally named "name" or "values" falsely triggers the duplicate-name error, and patchset["name"] returns {} instead of raising InvalidPatchLookup. Initialize to {}.

9. assert used for input/model validation — modifiers/histosys.py:107, modifiers/normsys.py:75 (user-supplied interpcodes; exceptions.InvalidInterpCode exists for exactly this), and modifiers/staterror.py:122 (guards the mask-equality invariant that the later [-1] indexing relies on). All vanish under python -O, turning bad input into silently wrong results.

Minor

10. src/pyhf/pdf.py:990-995 — the logpdf exception handler logs tensorlib.tolist(data); if astensor(data) was what raised (ragged input), tolist on the raw input can raise again inside the handler and mask the original error.
11. src/pyhf/infer/test_statistics.py:19 — docstring says "is 0 if muhat > 0" — should be "muhat > mu".
12. src/pyhf/optimize/mixins.py:190-192 — mutates pdf.config.par_names in place. Safe for pyhf models (the property rebuilds the list per access) but corrupts any duck-typed config that returns a stored list; copy first.

Performance

13. src/pyhf/workspace.py:54-79 (_join_items) — repeated secondary_item[key] in keys and double keys.index(...) make joins O(n·m); the comment even admits it. A {key: index} dict makes it linear; note keys is also never updated after appends, which only stays correct because workspace specs forbid duplicate names.
14. src/pyhf/modifiers/shapesys.py:152-159 / staterror.py:173-180 — default_backend.astensor(self._mask) re-tensorizes the entire (mods × samples × bins) mask inside the per-systematic loop at model build; hoist it out.
15. src/pyhf/modifiers/shapesys.py:120-132 — self.__shapesys_info builds a full (mods, samples, 3, bins) tensor that is never read anywhere (grep-verified). Delete it; it's both dead code and a build-time cost.
16. src/pyhf/modifiers/staterror.py:92-106 — per-(sample, bin) Python loop computing relerrs over already-numpy arrays; vectorize with np.where.
17. src/pyhf/modifiers/shapefactor.py:174-180 — element-wise triple loop doing single-element numpy assignments to populate the access field; vectorizable per (systematic, batch).
18. src/pyhf/parameters/paramview.py:~90 — _precompute re-tensorizes index arrays with the default float dtype although they were built as dtype="int", forcing int re-casts in every consumer (constraints, shapesys/staterror reindex). Pass dtype="int".

Simplifications

19. Modifier duplication (the biggest cleanup opportunity): normfactor_builder, shapefactor_builder, and lumi_builder are byte-identical except for the required_parset they call; shapesys_combined and staterror_combined share ~80 near-identical lines (__init__, _reindex_access_field, _precompute, apply); histosys/shapesys/staterror share the concatenate-and-validate finalize block. A small shared base class would remove several hundred lines. While at it, unify the inconsistent (batch, npars) if batch else (npars,) vs (batch or 1, npars) parfield-shape conventions.
20. Interpolators: _precompute_alphasets in code1.py and code4.py duplicates _precompute verbatim, and code4's six-row A_inverse matrix literal appears twice — the _slow_code4 copy rebuilt on every scalar call. (Agent-reported with quoted code; spot-checked pattern, not line-by-line.)
21. src/pyhf/tensor/manager.py:157-167 — bitwise | between boolean comparisons wrapped in bool(...); use or.
22. Small dead code: constraints.py:151 self.par_indices is never used (the gaussian twin doesn't have it); modifiers/shapesys.py:~190 calls get_backend() twice back-to-back; modifiers/normfactor.py:102-111 duplicates self.param_viewer.get(pars) in both branches (only the einsum subscript differs).
23. Typos worth fixing while touching pdf.py: _prameter_objects (line 169), finalizd_builder_data (line 151).

Modernizations

24. Python version window (pyproject.toml:14,35, noxfile.py:7): requires-python = ">=3.9" but 3.9 EOL'd October 2025; the noxfile still lists 3.8, which is below even the declared floor; classifiers stop at 3.13 with no 3.14 (released October 2025). Per SPEC 0, NumPy ecosystem packages would be on >=3.11 by now. Bumping the floor would also unlock dropping some from __future__ import annotations workarounds and older typing fallbacks.
25. src/pyhf/schema/validator.py:82 — still on the deprecated jsonschema.RefResolver (warning suppressed in pyproject.toml:182, tracked as issue jsonschema v4.18.0 will introduce breaking changes #2139). Migrating to the referencing registry API and bumping the jsonschema>=4.15 floor to >=4.18 closes that out.

The dependency floors, JAX usage (jax.jit/value_and_grad), iminuit v2 API, and importlib.resources patterns all checked out as current — no other modernization debt found.

If you'd like, I can turn the high-confidence, low-risk subset (items 1–9, 13–15, 21–23) into a fix branch with tests.

[henryiii]

🤖 AI text below 🤖

All 10 draft PRs are open against main, each referencing #2706, with passing targeted tests and clean lint:

PR	Title	Model
#2707	fix: skip and flush garbage-collected event callbacks (+ or cleanup in manager.py)	Opus
#2708	fix: optimizer post-processing robust to corr without unc	Opus
#2709	fix: patchset lookup keys, logpdf error logging, typos, docstring	Sonnet
#2710	perf: linear-time workspace joins	Sonnet
#2711	fix: file handles in readxml filecache + CLI patch loading	Sonnet
#2712	fix: paramset requirement reduction + constraint logpdf fallback	Opus
#2713	refactor: interpolator dedup (shared precompute, A_inverse factored)	Opus
#2714	fix: typed exceptions instead of asserts in modifier validation	Sonnet
#2715	perf: dead work removed + loops vectorized in modifier construction	Opus
#2716	refactor: jsonschema RefResolver → referencing (also resolves #2139)	Opus

A few things worth knowing:

• Pre-existing failure on main: the perf: remove dead work and vectorize loops in modifier construction #2715 agent ran the full fast suite and found 7 tests in tests/test_infer.py failing on clean main (divide-by-zero at src/pyhf/infer/calculators.py:441) — unrelated to any of these PRs, but something you'll want to look at separately.
• uv run is broken in this repo: several agents hit an unrelated resolution conflict (sphinx>=9.0.3 vs requires-python>=3.9 in the docs deps) when uv run re-resolves; they worked around it with uv pip install --group dev -e . + the venv's pytest directly. This is the Python-3.9-floor issue surfacing in practice — relevant if you revisit the version bump you deferred.
• Regression tests were confirmed to fail before each fix where applicable (fix: skip and flush garbage-collected event callbacks #2707, fix: make optimizer post-processing robust to correlations without uncertainties #2708, fix: correct paramset requirement reduction and constraint logpdf fallback #2712). The refactor: migrate from jsonschema.RefResolver to referencing #2716 migration had two non-obvious gotchas (draft-06 $ref/$id sibling suppression, and the cache-size test requiring a non-caching retrieve callback) — both explained in its PR body.
• As decided, the large modifier base-class dedup and the Python-floor change were left out; both are documented in the review summary if you want them noted on pyhf review from Claude Fable #2706.

[henryiii]

Do you need to regenerate image with Matplotlib 3.11, perhaps?