[DickyChant]

Fixes items 1, 2 and 6 of #200 in the datacard converter (rabbit_text2hdf5), with its first tests. Items 3–5 are left for a follow-up: 4 and 5 (rateParam initial value and range, param lines) need TensorWriter support and a design decision, and 3 (formula and root:ws rateParams) needs expression evaluation.

Changes (rabbit/datacard_converter.py)

1. Counting bins use the process rates. A bin without a shapes line gave every process the bin's observation as its yield; DatacardParser.rates was parsed but never used. Each process now takes its rate, and only data_obs takes the observation. The card in rabbit_text2hdf5: counting-experiment yields, rateParam wildcards and param lines in the datacard converter #200 now converts to SR: sig 10, bkg 100 and CR: bkg 1000 (was 160, 160, 1500).
2. rateParam bin and process fields are fnmatch patterns, as in Combine. A bin in which the process does not exist is not affected. It raises only if no bin matching the pattern has a matching process (was: KeyError for a process missing from a bin, and patterns other than * taken literally).
3. Explicit rates in shape cards are checked against the template integral with Combine's tolerance, |integral - rate| > 0.01 * max(1, rate) → ValueError("Mismatch in normalizations ..."). -1 still means "take the integral" (was: any rate silently ignored).

Tests (tests/test_datacard_converter.py, new)

Small counting and shape cards written on the fly, the shape templates in a ROOT file made with uproot:

• counting yields and observations;
• rateParam wildcards, skipping bins without the process;
• a rateParam that matches nothing raises (two cases);
• rate check: a mismatch raises, while -1 and a matching rate pass;
• rabbit_text2hdf5 on the command line, end to end.

8 pass on this branch. 6 of them fail on main; the other 2 cover cases that were already right.

Notes for review

• Dependencies. datacard_converter.py imports tqdm unconditionally, but tqdm is not in pyproject.toml; this predates this PR. The new test also needs uproot (the conversion extra). Locally the tests ran with both added through uv run --with tqdm --with uproot. If the CI runner lacks either, this file fails at collection. Adding tqdm to the dependencies could be a one-line follow-up.
• Zero rates. The rate check is skipped for rate <= 0, but a zero-rate process with a template still enters with the template's yield. Combine drops zero-rate processes. Left as is, as a possible follow-up.

Lint: black, isort and flake8 with the CI's --select list are clean.

Written with DeepSeek (dsh-tui), reviewed and tested by Claude Code.

Refs #200

🤖 Generated with Claude Code

https://claude.ai/code/session_01U7Dn1MUu97igKuLKFW9exA

[github-actions]

Issues Found

Critical (P0/P1)

• [P1] Apply the rateParam to every matched process, not only the first (rabbit/datacard_converter.py:315-325)
  • When an fnmatch process pattern matches more than one process in the same channel (e.g. r rateParam ch bkg* 1.0 with bkg1 and bkg2 in ch, or r rateParam ch * 1.0), this passes the whole matched list to TensorWriter.add_norm_systematic together with the scalar kappa. That writer wraps the scalar into a one-element list and iterates for p, u in zip(process, uncertainty) (tensorwriter.py:745), so the zip truncates to a single pair and only the first matched process receives the unconstrained rate scaling; all other matching processes silently get no rateParam, despite the writer's own error text stating a scalar uncertainty applies to every process in the list. Before this change such multi-process patterns failed loudly (the literal pattern was looked up in dict_norm and raised a KeyError), whereas now the card converts successfully and produces a silently wrong model; the new tests only exercise patterns matching a single process per channel, so this is not caught. Book the systematic per matched process instead.

Summary

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

Overall Verdict

Status: Patch has blocking issues

Explanation: The counting-bin rates and shape-card rate check are correct against the parser and writer contracts, and the unmatched-rateParam error is intentional. However, the new fnmatch process matching passes a multi-process list with a scalar kappa to TensorWriter.add_norm_systematic, which zips the two lists and therefore silently applies the rateParam to only the first matched process in each channel — cards that previously failed loudly now convert to silently wrong models.

Review details

• Model: cern/Kimi-K3-FLATIRON

Review metrics: 17 turns · 22 tool calls · 12m 23s

• Tokens: 534.3K input · 484.0K cached (91%) · 20.9K output

[cmsmwbot]

Bot review seat (auto-review per owner directive; head 76e6d43). Static review of the diff against the PR claims, plus verification of the hodor findings. This does not replace human review.

Claim-by-claim check of the diff

1. Counting bins use process rates (datacard_converter.py:271-281) — expected and understood. The counting branch now takes parser.rates[(bin_name, process_name)] for MC processes and keeps observations only for data_obs. Verified against the parser: bin_process_map/rates are built strictly from the process/rate card lines (datacard_parser.py:152-158), so data_obs can never appear as a key and the direct rates[...] lookup is safe for well-formed cards. The old behavior (every process gets the bin's observation) is visible in the diff, and the card from #200 indeed goes 160/160/1500 → sig 10 / bkg 100 / CR bkg 1000, exactly as test_counting_experiment_uses_process_rates asserts.

2. fnmatch rateParams (datacard_converter.py:309-329) — expected and understood, modulo the P1 below. Per-channel expansion over bin_process_map[channel] is the right semantics (a bin where the process does not exist is simply unaffected), and — important — a * process pattern cannot match data_obs, since data_obs is not in bin_process_map. The raise-on-no-match (ValueError at 325-329) is deliberate per the description, and both no-match cases are tested. Matches Combine's wildcard treatment as described.

3. Shape-card rate check (datacard_converter.py:255-269) — expected and understood. rate > 0 is checked against the bin-content sum of the template with tolerance 0.01 * max(1, rate); rate == -1 and rate <= 0 are skipped, consistent with the documented zero-rate caveat left as follow-up. The bin-content sum is the correct quantity to compare against a Combine rate line.

hodor review: CONFIRM the P1, with an augmentation

Hodor's blocking finding on the fnmatch/multi-process path is correct — I verified it in tensorwriter.py:734-746:

• A scalar uncertainty is wrapped as [kappa] (734-735), so the guard len(uncertainty) != 1 and ... (737) passes for any number of processes.
• for p, u in zip(process, uncertainty) (746) then iterates once — with the new code at datacard_converter.py:315-324, a pattern matching ≥2 processes in one channel (e.g. r rateParam ch bkg* 1.0 with bkg1, bkg2) silently books the rateParam on the first process only. The new tests only exercise single-match patterns (WILDCARD_CARD has one sig in ch1 and one bkg per channel), so it is indeed uncaught.

Augmentation, two nuances:

• Hodor's "previously failed loudly" holds for literal patterns (old code looked the literal bkg* up in dict_norm → KeyError), but the old *-path was already silently wrong in exactly the same way: processes = self.parser.processes (the whole card list) with the same scalar kappa truncated to the first process in card order. This PR generalizes the exposure from * to any multi-match pattern; the real defect is the zip-instead-of-broadcast in TensorWriter.add_norm_systematic, whose own error text implies scalar-means-all-processes. Hodor's recommended fix (book the systematic per matched process, one add_norm_systematic call each) resolves both generations; alternatively broadcast the scalar inside the writer and add a regression test there.
• The writer defect is worth tracking beyond this PR regardless of how the fix lands here, since other callers may assume scalar-broadcast semantics.

CI / validation products

• Gate jobs on the linked CI run are green so far: linting, setenv, make-tensor, make-abcd-tensors, symmerizations — so the CI card corpus still converts with the new counting/rate logic. all-unit-tests is still running as I write this; its outcome for tests/test_datacard_converter.py is the open dependency question the PR itself flags (unconditional import tqdm in datacard_converter.py and import uproot in the test; tqdm is not in pyproject.toml). A collection failure there would show as a per-file error in that job.
• No EOS validation products are available for this PR: copy-clean has failed on every run since the lxplus8s10 reboot on Nov 4 wiped the runner's ambient kerberos ccache (/eos/user/c/cmsmwbot/www/Rabbit/PRValidation/PR220/ does not exist). That outage and its fix are #221; this review is therefore log- and static-analysis-based. For this diff that is proportionate: the changed code paths are unit-tested logic, not numerics that shift validation histograms.

Watch before merge

1. The hodor P1 (multi-process rateParam patterns) — fix or an explicit decision to defer with a tracking issue.
2. all-unit-tests completing green for the new converter tests in the CI environment (tqdm/uproot).
3. copy-clean on #221 to restore EOS products for future PR comparisons.

[github-actions]

Issues Found

No issues found.

Summary

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

Overall Verdict

Status: Patch is correct

Explanation: The incremental delta only swaps a top-level import uproot in tests/test_datacard_converter.py for pytest.importorskip("uproot"). The placement is safe because rabbit/datacard_converter.py imports uproot lazily inside a method, so the converter import cannot raise before the skip takes effect, and the bound uproot name still covers the single later uproot.recreate use. Module-wide skipping is the documented intent, so no findings qualify.

Review details

• Model: cern/Kimi-K3-FLATIRON

Review metrics: 2 turns · 3 tool calls · 19s

• Tokens: 10.7K input · 4.7K cached (44%) · 1.8K output

[DickyChant]

The all-unit-tests failure here is not in the fixes themselves. The CI runner has neither dependency that tests/test_datacard_converter.py needs:

• tqdm: rabbit/datacard_converter.py imports it at module level, but pyproject.toml does not declare it, not even in the conversion extra. So pip install rabbit-fit[conversion] gives a rabbit_text2hdf5 that fails to import unless tqdm happens to be installed.
• uproot: declared in conversion, but the runner uses its system Python and does not install the extras.

Before we implement anything, I think we should decide how to handle these:

1. tqdm: make it optional in the converter (it only draws two progress bars), or declare it, e.g. in conversion.
2. uproot on the runner: if it is installed there, the converter tests run in CI with no workflow change. The alternative is pytest.importorskip, which keeps CI green but means the converter is still never tested in CI, the gap rabbit_text2hdf5: counting-experiment yields, rateParam wildcards and param lines in the datacard converter #200 points out.

@davidwalter2, what do you prefer, especially for 2? I have the importorskip version locally but have not pushed it.

[davidwalter2]

Thanks Sitian. For both I'd go for the variant that tests the converter in CI:

1. tqdm: please declare it in the conversion extra, next to uproot.
2. uproot on the runner: uproot and tqdm are now installed in the CI venv, so the converter tests run there. (The runners use the venv ~/CombineTF2CI/env on lxplus8s10, not the system Python.) Please drop the importorskip and go back to a plain import uproot, so that a runner without it fails instead of skipping the tests. 18e6c69 with the importorskip is already on the branch, but it never took effect: it comes after from rabbit.datacard_converter import DatacardConverter, and that import is what failed on tqdm in the Nov 7 run.
3. Hodor's P1 needs a fix before we merge. A rateParam pattern that matches several processes in one channel (bkg*, or *) is booked on the first one only. Please pass [self.rate_param_kappa] * len(processes) from the converter, and add a test with two matched processes in one channel. The root cause is TensorWriter.add_norm_systematic, which wraps a scalar into a one-element list and then zips it with the processes. That also hits tests/make_tensor.py:335, where norm in ch0 only reaches sig. Fixing the writer changes the CI test tensor, so it goes into a separate PR; with the explicit list the converter is right either way.

The zero-rate follow-up can stay out of this PR, as you suggested.

[github-actions]

Issues Found

No issues found.

Summary

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

Overall Verdict

Status: Patch is correct

Explanation: The delta fixes the previously reported P1 exactly as agreed in the MR notes: the converter now passes one kappa per matched process, which satisfies TensorWriter.add_norm_systematic's scalar-wrap-and-zip contract so every fnmatch-matched process is booked. The new test's expectations (np.log(1.01) per matched process under the writer's default log_normal type, none on the unmatched process) are consistent with the writer and parser contracts, and declaring tqdm in the conversion extra matches its module-level import in the converter.

Review details

• Model: cern/Kimi-K3-FLATIRON

Review metrics: 6 turns · 9 tool calls · 2m 9s

• Tokens: 72.5K input · 54.8K cached (76%) · 7.1K output

[cmsmwbot]

CI status — workflow Hodor AI review, run 42665043962: ✅ 1/1 jobs green (slowest/only review, 2m43s). The main CI workflow for head bfebb1ac is still in progress — one consolidated note when it finishes.