[DickyChant]

TensorWriter.add_norm_systematic fails for asymmetric lnN uncertainties unless symmetrize is "average" or "conservative":

w.add_norm_systematic("x", ["bkg"], "ch0", [(1.05, 0.97)], symmetrize=None)
# TypeError: unhashable type: 'list'
#   add_norm_systematic -> _compute_asym_syst -> book_logk_halfdiff -> book_logk

Cause. Inside the for p, u in zip(process, uncertainty) loop, both the dense branch and the sparse-norm branch pass the whole process list to _compute_asym_syst instead of p.

• "average" and "conservative" only return values, which are then booked per p, so they work.
• None, "linear" and "quadratic" book inside _compute_asym_syst and fail, even for a single process.

Fix. Pass p in both calls.

Also in this PR. For a single process, given as a name or as a one-element list or tuple, a flat [up, down] (e.g. add_norm_systematic("x", "bkg", "ch0", [1.05, 0.97])) was read as two per-process values and rejected by the length check. It is now taken as one asymmetric uncertainty. The one-element list form was added in b6e070b, following the review. Previously this form always raised, so no working call changes meaning. The existing forms are unchanged: scalar, [k], and [(up, down)] (the one datacard_converter uses).

Tests. New tests/test_norm_systematic_asym.py, covering:

• every symmetrize mode, two processes with different asymmetric uncertainties, dense and SparseHist inputs;
• per process, the booked logkavg / logkhalfdiff / SymDiff values against the expected values;
• the single-process forms.

21 passed. Without the fix, the None / "linear" / "quadratic" cases fail with the TypeError above.

test_datacard_converter.py, test_logk_no_copy.py, test_chi2_singular.py and test_multi_systematic.py pass locally. test_sparse_fit.py errors in my local environment with or without this change: Python 3.9 limits it to scipy 1.13, which lacks 1-D sparse arrays.

Reported by a VBS WZ polarization analysis using asymmetric normalization uncertainties.

🤖 Generated with Claude Code

https://claude.ai/code/session_01F1LSAyy45b9quu3jDVBkcn

[cmsmwbot]

Bot review (auto-triage seat on this PR; content-level, does not replace human review — COMMENT-only per policy)

I verified the claims directly at head 3aeb898:

1. Root cause — CONFIRMED. On main, the PR's minimal example reproduces the failure:
add_norm_systematic("x", ["bkg"], "ch0", [(1.05, 0.97)], symmetrize=None) → TypeError: unhashable type: 'list'.
Mechanism matches the description: in the for p, u in zip(process, uncertainty) loop, tensorwriter.py passed the whole process list into _compute_asym_syst, which books per process (book_logk_avg(..., process, var_name_out_diff) for linear/quadratic, book_logk_halfdiff(..., process, name) for None) → the list landed in dict lookups. "average"/"conservative" only compute return values, so they were unaffected. Both the dense branch and the SparseHist branch had this bug; both are fixed by passing p. book_logk_avg(logkavg_proc, channel, p, var_name_out) in add_norm_systematic was already per-process.

2. New tests — PASS here, expectations CHECKED against the formulas. tests/test_norm_systematic_asym.py (15 cases) passes at this head (python3.9, dense + SparseHist). The expected values match _compute_asym_syst exactly: avg = 0.5·(logkup+logkdown), halfdiff = 0.5·(logkup−logkdown), SymDiff scaled by √3 for quadratic — with logkdown = −log(down) as the code computes it. No discrepancy between test and implementation.

3. Single-process flat [up, down] form — claim CONFIRMED, no existing call changed. The wrap to [(up, down)] triggers only for a bare process name (np.ndim(uncertainty)==1 and len==2), and test_single_process_existing_forms pins the existing forms: scalar and [k] → logkavg = log(k) (symmetric); [(1.05, 0.97)] with default symmetrize="average" → 0.5·log(1.05/0.97), i.e. datacard_converter's combine-style convention is untouched.

Observations (non-blocking)

• The flat-pair convenience is only recognized for a bare string process; ["bkg"] / ("bkg",) with [1.05, 0.97] still raise the length RuntimeError. Consistent with the PR description ("single process name"), but one-element-list users may reasonably expect the same — worth either accepting or mentioning explicitly in the error.
• The fixtures contain a zero-norm bin, exercising the pre-existing invalid value encountered in divide RuntimeWarning in get_logk; the booked values there are 0 (which the tests' values != 0 helper implicitly guards). Not introduced by this PR.

Other automated signal

• CI green so far: setenv, linting, make-tensor, make-abcd-tensors, symmerizations. all-unit-tests is still running at posting time (whole suite in a single step).
• The hodor review (check run review) is likewise still in progress, so there are no conclusions to confirm/augment/correct yet — the code-level verification above was done independently. Will compare once it lands.
• No EOS plot validation applicable (rabbit CI runs tensor/unit jobs; this change is norm-systematic booking, covered by the unit tests).

Watch next: all-unit-tests (in particular test_datacard_converter.py, test_logk_no_copy.py, test_multi_systematic.py) and hodor's summary; if either flags a regression somewhere the new file doesn't cover, I'll follow up in-thread.

[github-actions]

Issues Found

No issues found.

Summary

Total issues: 0 critical, 0 important, 0 minor.

Overall Verdict

Status: Patch is correct

Explanation: The fix correctly passes the per-process loop variable p (instead of the whole process list) to _compute_asym_syst in both the dense and sparse-norm branches; _compute_asym_syst's process argument is used as a dict key by book_logk, so the list was unhashable for the None/linear/quadratic modes that book inside it. The new flat [up, down] wrap for a single process only rescues inputs that previously raised RuntimeError in the length validation, leaving all existing valid forms unaffected, and the new tests' expected values match the implementation formulas across all symmetrize modes and both storage paths.

Review details

• Model: cern/Kimi-K3-FLATIRON

Review metrics: 8 turns · 10 tool calls · 3m 51s

• Tokens: 130.8K input · 100.9K cached (77%) · 12.1K output

[cmsmwbot]

CI status — workflow Hodor AI review, run 42744644389: ✅ 1/1 jobs green (slowest/only review, 4m19s) on b6e070b. The main CI run 42744646703 is still running (5/6 jobs green so far, all-unit-tests in progress) — I'll post its consolidated status when it finishes.

[cmsmwbot]

✅ CI green at b6e070b — 19/19 jobs passed (wall time ~63 min).

Slowest jobs: all-unit-tests (56m), alternative-fits (bb_full_gamma) (4m), fitting (3m).

Ready for review.