[vslokenb]

Follow-up to issue #228 . Added new paramModel class, example case is VBS WV xsec measurement (need both WW and WZ for sensitivity). Previously, a datacard with multiple signals would try to fit each process separately.

[github-actions]

Issues Found

Important (P2)

• [P2] Reformat SharedMu so the CI black --check step passes (rabbit/param_models/param_model.py:390-391)
  • The repo CI (.github/workflows/main.yml) runs black --check ., which fails deterministically on this insertion: there are no blank lines (black requires two before top-level definitions) between the end of Mu.compute's body and the ## CUSTOM comment / class SharedMu, only one blank line now remains before class Mixture (origin had two), and several added lines exceed the 88-char line length (e.g. the new init signature at line 409 is 98 chars, lines 424 and 429 are 91) which black will rewrap. Running black rabbit/param_models/param_model.py and committing the result is required before this PR can pass CI.

Minor (P3)

• [P3] Add a test exercising SharedMu's mask and rate scaling (rabbit/param_models/param_model.py:436-439)
  • This new model derives a per-process mask from indata.procs/indata.signals ordering, parses comma-separated CLI tokens, and computes the per-process rates consumed by the fitter, but no test references SharedMu. A regression in the mask construction or reshape (e.g. wrong target set or process ordering) would silently produce scaled yields on the wrong processes rather than fail. A small test in the style of the existing load_model("Mu", ...) fit tests that loads a two-signal tensor and asserts r scales exactly the tied processes would lock the contract.

Summary

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

Overall Verdict

Status: Patch has blocking issues

Explanation: The SharedMu model logic itself is consistent with the existing param-model contracts (squaring transform, [1, nproc] output, param naming, CLI registration), but the insertion breaks the enforced black --check CI step by dropping required blank lines and adding over-length lines, and the new behavior ships without any test coverage.

Review details

• Model: cern/Kimi-K3-FLATIRON

Review metrics: 20 turns · 30 tool calls · 4m 34s

• Tokens: 450.3K input · 406.4K cached (90%) · 20.3K output

[cmsmwbot]

cmsmwbot self-review — static, COMMENT-only (no CI validation products exist yet; see CI caveat at the end).

Claim checked: per the description (#228), a shared signal strength for multiple signal processes (VBS WV-style: WW+WZ under one r), where the existing Mu model fits each signal separately. The diff delivers exactly that: SharedMu.npoi = 1 with a per-process mask vs. Mu.npoi = indata.nsignals; compute() returns 1 + (r−1)·mask → r on tied processes, 1 elsewhere, reshaped [1, nproc] per the model contract.

Correct-by-construction (verified against Mu/ParamModel):

• Ordering-independent mask via np.isin(indata.procs, targets) — strictly safer than Mu.compute()'s positional concat, which assumes signals precede non-signals in indata.procs.
• Interface conformance: npoi/npou/params/xparamdefault/is_linear/allowNegativeParam all set exactly as the base class contract requires; params = [b"r"] bytes matches set_param_default()'s signal.encode() comparison, so --expectSignal r=<val> works with the POI literally named r.
• parse_args caps at one comma-list argument with a clear ValueError; missing-file→ValueError and empty-target guards raise before any tensor is built. Ignoring full is safe here (npou = 0, so there is no POI/POU split to distinguish).

Hodor cross-check (hodor review completed, conclusion success; findings verified independently):

• CONFIRM — [P2] black: I ran the CI lint step on this head (black --exclude '(^\.git|\.github)' --check ., black 25.1.0): rabbit/param_models/param_model.py fails — no blank lines between Mu.compute's end and the ## CUSTOM banner (black wants two before the top-level class), one blank line before class Mixture where the original had two (the insertion consumed one), and the 98-char __init__ signature plus two 91-char lines need rewraps. black rabbit/param_models/param_model.py resolves it. (My run additionally flags 4 files this PR doesn't touch — black version drift on my side vs. the runner image; the findings on the touched file are version-stable.)
• CONFIRM — [P3] test coverage: grep -rl SharedMu tests/ is empty; tests/test_param_model_initial_params.py and tests/test_saturated_composite.py are the closest templates. A small two-signal tensor asserting r scales exactly the tied processes would lock the mask contract — worth adding given nothing else exercises this model in this PR.

AUGMENT (nit, non-blocking): the ## CUSTOM FOR MULTIPLE SIGNAL PROCESSES SHARING 1 SIGNAL STRENGTH (ex. VBS WV) banner reads like developer-local markup — suggest folding the VBS WV motivation into the class docstring and dropping the banner.

CI caveat: the CI workflow run for this head is parked at action_required (fork PR awaiting maintainer approval), so no run of lint/tests and no PRValidation products exist yet; the checks tab does not yet show the black failure above. Once the run is approved, expect black --check to fail as described, and I'll compare the validation output when it lands.

Verdict: logic is expected-and-understood for the multi-signal shared-strength use case; only the black formatting blocks. Hodor's conclusions are confirmed with no corrections.

[cmsmwbot]

CI status at be48ae82:

• ✅ Hodor AI review — run 42758073561: 1/1 jobs green (only review, 5m1s).
• ⚠️ CI — run 42758076529: finished as action required — no jobs ran. This workflow is gated on maintainer approval for fork pull requests; a maintainer needs to approve the run ("Approve and run" on the Actions page) before the CI can execute. I'll report the consolidated CI status once it completes.