fix(lookup_tools): convert_histo_json_file uses current histogram, not stale loop variable

[NJManganelli]

Part of #1578 — critical bug 8: json_converters.py uses stale loop variable.

convert_histo_json_file converts JSON scale-factor files to dense-lookup tables in a two-pass structure. The second pass looked up the set of value names via names_and_valnames[histname], but histname is a stale loop variable from the first pass that always points at the last histogram in the file. Every histogram in a multi-histogram JSON was therefore assigned the last histogram's value names — silently dropping value tables when a histogram had extra names, or raising KeyError when it had a name the last histogram lacked, depending on key ordering. Single-histogram files (like the existing test sample) never triggered it.

The fix replaces histname with the current loop variable name, matching the value lookups a few lines below that already used name correctly. A tiny multi-histogram sample (tests/samples/multihist_WH_out.histo.json) and a regression test are added; the test fails with KeyError before the fix and passes after, and the full tests/test_lookup_tools.py suite (15 tests) passes. Pre-commit clean.

🤖 Generated with Claude Code

[lgray]

🤖 AI text below 🤖

Reviewed on behalf of Lindsey Gray.

Summary / assessment: This is a correct, well-targeted one-character fix. In convert_histo_json_file, histname is bound only in the first pass (json_converters.py:66) and, because Python leaks loop variables, it holds the last histogram's key by the time the second pass runs. The second pass (line 73 onward) otherwise keys everything on the current name (names_and_orders[name], names_and_binvalues[name], output on line 92) — line 76 was the lone straggler still reading names_and_valnames[histname]. Switching it to [name] makes the value-name lookup consistent with the sibling lookups. I traced every use of histname and the names_and_* dicts and confirmed this is the only stale-variable read in the function — the fix is complete, not partial. Recommend merge.

I verified the fix is discriminating: reverting only the source to master and rerunning the new test fails with KeyError: 'weight' at json_converters.py:86 (histogram dirA/histA, which lacks the weight table the stale last-histogram key demanded); restoring the fix passes. Full tests/test_lookup_tools.py → 10 passed, 5 skipped (dask_awkward-guarded, unrelated).

Bugs/Blockers: none.

Should fix: none.

Nits/Optional:

• The test exercises the KeyError mode but not the silent-drop mode, which is the more dangerous of the two. When the last histogram's value-names are a subset of an earlier histogram's, the buggy code raises nothing and silently omits value tables. I reproduced this on master with a variant sample (dirA={value,error}, dirB={value}): master drops dirA/histA_error with no error; the fix restores it. Both modes share the same root cause and line, so the current test already guards the fix — but a second histogram ordering where the last hist is a strict subset would document the silent mode too. Optional.
• Optional hardening against recurrence: histname is genuinely needed in the first pass, but you could drop the leak entirely by keying the second-pass loop off the same names, e.g. iterate for name in names_and_valnames: (already effectively what happens). Not worth a change on its own; the current fix is fine.

Test coverage: Good and non-vacuous. The new sample's two histograms have genuinely different value-name sets ({value,error} vs {value,weight}), which is what makes the bug fire — confirmed by the revert producing KeyError: 'weight'. The test asserts both the exact resolved key set and the per-table dense-array contents ([1.0,2.0], [0.1,0.2], [5.0,6.0], [9.0,8.0]) plus an evaluator round-trip, so it catches wrong/mismatched data, not merely "runs without exception." The sample is minimal, well-formed, matches the histo.json schema, and is clearly synthetic despite the WH name (no provenance/license concern).

Pydantic: Not applicable — the diff contains no pydantic usage (grep: 0 hits).

[NJManganelli]

Did we want the optional extension flagged in review, or good to go?

[NJManganelli]

Anything else needed here?