Skip to content

scetlib_ad: QED ISR order-step nuisances on the AD tape (qedStep_*), NLL' central recommended - #770

Open
lucalavezzo wants to merge 8 commits into
mainfrom
ad-qed-kappa
Open

lucalavezzo wants to merge 8 commits into
mainfrom
ad-qed-kappa

Conversation

@lucalavezzo

@lucalavezzo lucalavezzo commented Nov 12, 2026 •

Copy link
Copy Markdown
Collaborator

What

QED ISR order-step nuisances for SCETlibADParamModel (#739), evaluated on the AD tape, plus the frozen-template hooks they replace and two small theory-helper fixes.

  • qedStep_* nuisances. A SCETlib cache built with [QED] qed_kappa = yes registers one multiplier qed_kappa_<step> per QED order step present (ll: off→LL, nll: LL→NLL, nllp: NLL→NLL′, mix: NLL′→NNLL mixed QCD×QED), central 1. The param model exposes them as qedStep_<step> = θ = κ − 1, an N(0,1) unit nuisance (θ = −1 is the next-lower order exactly), all in the qedOrder impact group. The response is the calculation's own, cross terms with the NP λ, α_s, TNPs and PDFs included.
  • qed_nuisances = top2 | all | none | <step list> (model arg). top2 (default) floats the two highest steps below the cache's central order: NLL′ → nll, nllp; NNLL → nllp, mix; NLL → ll, nll. Floating ll (the whole LL effect, degenerate with α_s) warns. Refused: together with qed_knob, with priors off, or with fit_params contradicting the rule.
  • [QED] refuse check. [QED] now takes part in the cache-vs-theory-correction config check (qed_isr, mixed_order, qcd_side_mixed, mixed_running, alpha_fo, alpha_mz, mz), compared on effective settings with absent = off / the SCETlib default, so a QED-on cache under a QED-off correction no longer passes silently. Escapes: the legacy frozen-K workflow (QED-off correction + QED-on cache + fixed_ratio) is a warning, so existing configurations replay unchanged; qed_check=warn downgrades any QED mismatch. fixed_ratio on a QED-on correction refuses (K applied twice).
  • pythiaew_ISR conflict. A card that still carries the --isrUnc (pythiaew_ISR) templates is refused when a qedStep_* is floated (double counting). Frozen-template fits are unaffected. The W default is not touched.
  • Hooks (frozen templates, kept for comparisons): fixed_ratio= (a fixed per-cache-bin factor K), qed_knob= (one or more frozen T^θ nuisances, group qedKnob), gen_absY_max= (synthetic |Y| bin for a qT-only gen card).
  • rabbit_theory_helper: the correction-hist separator is always _ (single-entry --theoryCorr lists and suffixed nominals such as ..._adcorr broke the old heuristic), and the pdfvars/pdfas sidecars of a suffixed nominal are found by stem (needed for cache-derived corrections and MSHT20 cards).

Recommended Z configuration (B′: NLL′ central)

[QED]
qed_isr = nllp
qed_kappa = yes
# mixed_order / qcd_side_mixed absent (off); mixed_running, alpha_fo at their defaults

with the default qed_nuisances=top2, which floats qedStep_nll and qedStep_nllp. The NLL′ central is the configuration cross-checked against DYTurbo; the docstrings now list it first. The NNLL central (mixed_order = nnll, qcd_side_mixed = yes, floating qedStep_nllp + qedStep_mix) stays supported. This block is now in the default Z runcard scripts/rabbit/scetlib_ad/conf/Z_CT18Z_N3p0LL_FranksVals.conf, so a cache built from it has QED ISR at NLL′ in the central prediction and top2 floats exactly these two steps (no param-model behaviour change was needed). Consequence: such a cache needs a scetlib-cms with QED ISR (MR !29), builds PDF-dependent, and must be paired with a theory correction carrying the same [QED] (cache_to_theorycorr.py does this); a QCD-only correction is refused by the [QED] check. Caches built earlier from the QCD-only runcard are unaffected.

Dependency

The cache side (qed_kappa, the order-step multipliers) is scetlib-cms MR !29: https://gitlab.cern.ch/scetlib/contrib/scetlib-cms/-/merge_requests/29. On a cache without the multipliers qed_nuisances is inactive (a note), and everything else behaves as before.

How tested

  • Unit tests (mock core, no cache), 77/77 pass in the container on this branch: params-level QED settings and the step table, the model on a mock κ cache (names, θ = κ − 1, priors, qedOrder group, selection per central order, refusals), the [QED] refusal and its escapes, and bit-identical parameter list / start vector / priors / compute() and gradient against the hook commit on a non-κ cache. The pythiaew_ISR refusal check passes. top2 on an NLL′ config returns (nll, nllp), on NNLL (nllp, mix).
  • Tape vs frozen templates (production QED+κ cache, NNLL central, CT18Z reco fit): Asimov σ(α_s) and the QED-order group impact identical to ≤ 1e-3 relative; on data Δα_s(tape − frozen) = −0.006e-3 (blinded difference), σ(α_s) identical, the same NP faces.
  • Start checks: an unblinded --noFit prefit with the param model reproduces the card without the param model to all digits (B′ card: prefit 2ΔNLL identical, nllvalreduced difference exactly 0); the [QED] check passes with 0 refusals and floats exactly qedStep_nll, qedStep_nllp.
  • B′ vs NNLL central (blinded data fit, same blinding offset): Δα_s = −0.025e-3 (0.023σ), σ(α_s) unchanged (−0.2 %), the same NP faces; the expected QED-order group impact grows from 0.078e-3 to 0.101e-3.
  • Lint: black / isort / flake8 (CI flags, CI image) clean on the changed files.

🤖 Generated with Claude Code

lucalavezzo and others added 7 commits November 12, 2026 09:36
…Y_max hooks

The frozen-template QED hooks used by the qed-isr study fits (261030 T2,
261103 D / B' / Bs / Cs): an optional fixed per-cache-bin factor K
(fixed_ratio=), one or more frozen T**theta nuisances (qed_knob=, group
qedKnob), and a synthetic |Y| bin for a qT-only gen card (gen_absY_max=).
Committed verbatim from the WRemnants-isr-fixed-gen-ratio worktree (diff
md5 bc8dbc297fb6, the one every scheme fit was launched with).

Co-Authored-By: Claude Opus 5.5 <[email protected]>
A cache built with [QED] qed_kappa = yes (scetlib-cms qed-order-kappa)
registers one multiplier qed_kappa_<step> per QED order step present
(ll: off->LL, nll: LL->NLL, nllp: NLL->NLLp, mix: NLLp->NNLL mixed),
central 1. The param model exposes them as qedStep_<step> = theta =
kappa - 1 (REPARAM unit, width 1; anchor 1 from structural_central;
N(0,1) prior), all in the qedOrder impact group.

qed_nuisances = top2 (default) | all | none | <step list>: top2 floats
the two highest steps below the central order the cache [QED] configures
(NNLL -> nllp, mix = the C-split scheme; NLLp -> nll, nllp; NLL -> ll,
nll; LL -> ll), the rest stay at kappa = 1. Floating ll (the whole LL
effect, degenerate with alpha_s) warns. Refused: together with the frozen
qed_knob templates, with priors off, fit_params contradicting the rule,
and a runcard whose [QED] does not imply the registered steps.

[QED] now takes part in the cache-vs-correction config check: qed_isr,
mixed_order, qcd_side_mixed, mixed_running, alpha_fo, alpha_mz, mz are
REFUSE keys, compared on EFFECTIVE settings with "absent" = off / the
SCETlib default on both sides (a pre-QED correction has no [QED]
section, so the generic loop never visited it and a QED-on cache under a
QED-off correction passed silently). qed_kappa is not compared (no
number changes at kappa = 1). Escapes: the legacy frozen-K workflow
(QED-off correction + QED-on cache + fixed_ratio) is accepted with a
warning, so every existing scheme-fit configuration replays unchanged;
qed_check=warn downgrades any QED mismatch. fixed_ratio on a QED-on
correction refuses (K applied twice) unless qed_check=warn.

fixed_ratio, qed_knob and gen_absY_max are unchanged.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
…QED order steps

A card that still carries the --isrUnc templates double counts the QED ISR
effect the qedStep_* nuisances already describe. The refusal applies only
when a qedStep_* is floated, so frozen-template (qed_knob) fits on existing
cards are unaffected.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
The histmaker writes the correction hist as `f"{generator}_Corr"`
unconditionally (production/systematics.py:706), so there is nothing to
infer. The heuristic this replaces guessed the separator from whether the
SECOND --theoryCorr entry was a prefix-extension of the first, which is
true for the shipped naming (nominal `..._N2LO`, variations
`..._N2LO_pdfvars`) but fails in two configurations we need:

  * a single-entry --theoryCorr list, i.e. passing only a central
    correction with no pdfvars/pdfas siblings -- `len(corr_hists) > 1` is
    False, so the separator came out empty;
  * a nominal carrying its own suffix, e.g. a cache-derived
    `..._N2LO_adcorrY4` alongside `..._N2LO_pdfvars` -- the prefix test
    fails for the same reason.

Both then resolve to `..._adcorrY4Corr`, which no histmaker emits, and
card building dies in loadHistsForDatagroups with "Did not find
systematic".

Note this drops support for reading hists whose name uses the
no-underscore form (`<generator>Corr`). Nothing in the current histmaker
can produce that; it only appears in archived outputs from before
systematics.py settled on `_Corr`, and in one legacy fallback literal
here.

Co-Authored-By: Claude Opus 5 <[email protected]>
…ominal

The sidecar names were derived by string surgery on the nominal correction
(<nominal>_pdfvars / <nominal>_pdfas). A nominal that carries its own suffix,
as every cache-derived correction does (..._N2LO_adcorr alongside
..._N2LO_pdfvars/_pdfas), then finds neither: pdfvars raised outright, and
pdfas fell back to a hard-coded CT18Z name and raised if that was absent too,
so an MSHT20 card could not be built.

A sidecar is now any theoryCorr entry <stem>_<kind> whose <stem> is the
nominal's name or a prefix of it, required to be unique. The exact name still
matches (stem == nominal), another set's sidecar (..._MSHT20mcrange_..._pdfvars)
can never match, and the CT18Z alphaS fallback is kept as the last resort.

Checked on the MSHT20 card: pdfvars/pdfas resolve to the MSHT20 sidecars and
the card's 3673 nuisances are identical to the CT18Z production card's.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
…guration

Docstrings only, no behaviour change. With [QED] qed_isr = nllp, qed_kappa =
yes and no mixed term (mixed_order / qcd_side_mixed off), the default
qed_nuisances = top2 floats qedStep_nll and qedStep_nllp. That NLL' central is
the configuration cross-checked against DYTurbo, so it is now listed first as
the recommended Z configuration; the NNLL central (nllp + mixed_order = nnll ->
qedStep_nllp, qedStep_mix) stays supported.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Add [QED] qed_isr = nllp, qed_kappa = yes to Z_CT18Z_N3p0LL_FranksVals.conf,
so a cache built from the default runcard carries QED ISR at NLL' in the
central prediction (the order verified against DYTurbo 1.4.2) and registers
the order-step multipliers; qed_nuisances=top2 then floats qedStep_nll and
qedStep_nllp (B'-split). mixed_order and qcd_side_mixed stay off.

Needs a scetlib-cms with QED ISR (MR !29). Theory corrections must carry the
same [QED]; a QCD-only correction on a cache from this runcard is refused by
the [QED] check.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01Sr6N7Ywi7iExTtmJDiju9W
@cmsmwbot

cmsmwbot Bot commented Nov 12, 2026

Copy link
Copy Markdown

CI status (run #4377, head df1cdee): all 25 jobs green ✅

  • Slowest: w-analysis 72m31s, lowpu-w 51m22s, \alphas-z-histmaker 49m30s; wall ~84 min
  • Prior head 86411b2 had failed (run #4375, copy-clean); this head is clean
  • Validation products: /eos/user/c/cmsmwbot/www/WMassAnalysis/PRValidation/PR770/

Status only — no review verdict implied. PR is still draft; flip to ready_for_review when you want full triage/review.

@lucalavezzo
lucalavezzo marked this pull request as ready for review November 12, 2026 21:56
@cmsmwbot

cmsmwbot Bot commented Nov 12, 2026

Copy link
Copy Markdown

cmsmwbot[bot] self-review: on it — reading the diff + checking hodor/CI and the PR-validation outputs for head df1cdee; full review note to follow.

@cmsmwbot cmsmwbot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cmsmwbot[bot] self-review — head df1cdee

Scope: the diff (+451 param_model.py, +266 params.py, +39/−14 rabbit_theory_helper.py, runcard, README) judged against the PR description, plus the PR-validation CI products compared against the latest scheduled main reference.

Code review

1. QED order-step machinery (params.py). qed_steps_from_config mirrors SCETlib's kappa registration (ll whenever QED on → +nll at ≥NLL → +nllp at NLL′ → +mix with mixed_order=nnll), and top2 = the last two present steps, which reproduces every row of the docstring table (NLL′→(nll, nllp); NNLL→(nllp, mix); NLL→(ll, nll); LL→(ll)). θ = κ−1 is handled consistently: structural_central(qedStep_*) = 1.0 is the κ-space central with REPARAM at the boundary, and the prior entry ("unit", (1.0,)) is exactly the claimed N(0,1). The refusals are each in the right place: explicit step list on a non-κ cache; κ-registry vs runcard disagreement; fit_params naming a step qed_nuisances doesn't select; qedStep_* with priors off; qed_knob together with any selected step (double count). Floating ll warns rather than refuses, which matches the "checks and bounds only" intent.

2. [QED] cache↔correction check (params.py + param_model.py). Comparing effective settings with absent = off/default closes a real hole: a correction made before [QED] existed carries no such section, so the old generic key-walk never visited the mismatch and a QED-on cache under a QCD-only correction passed silently. The per-key order gating (_QED_KEY_ORDERS) matches SCETlib's actual reads (e.g. mixed_order at qed_isr=ll is inert and doesn't poison the comparison). The two escapes are the right two and only those: the legacy frozen-K workflow (QED-off correction / QED-on cache / fixed_ratio) → warning; fixed_ratio on a QED-on correction → refusal (K applied twice). qed_check=warn downgrades both as documented.

3. pythiaew_ISR conflict (_CONFLICTS). Verified against scripts/rabbit/setupRabbit.py (--isrUnc → pythiaew_ISR): the regex ew.*_isr is matched case-insensitively against indata.systs (module comment), so it does catch pythiaew_ISR — worth noting explicitly since a case-sensitive read of the regex would suggest otherwise. The trigger condition (any fitted qedStep_*) correctly leaves frozen-template fits untouched, and the W default is not affected.

4. rabbit_theory_helper.py. The always-"_" separator: verified the histmaker writes f"{generator}_Corr" unconditionally (wremnants/production/systematics.py:706, also wremnants/production/histmaker_tools.py:193); nothing in the current tree emits XCorr, so the old sep == "" branch was dead weight that actively broke the two stated cases (single-entry --theoryCorr lists; nominals carrying their own suffix like ..._N2LO_adcorrY4). _sidecar_corr_hist's prefix-by-stem lookup: checked it cannot confuse MSHT20 with MSHT20mcrange/mbrange sidecars (the stems diverge at the suffix position), and failures are sane (ambiguous → warning + None; pdfas then falls back to the CT18Z pdfas hist, pdfvars raises). The leftover if self._corr_sep == "_" fallback in add_pdf_alphas_variation is now dead but harmless.

5. Frozen-template hooks. fixed_ratio (K) and qed_knob (T^θ): bin-by-value matching with round-6 keys and a missing-bin refusal; the knob θ indices sit after the envelope (i0 = n_scetlib + len(env)) consistently across _sigma_gen, _param_order, priors, and impact groups. The companion fix param[nsc:] → param[nsc:nsc+len(env)] in _ratio_from_param fixes a latent mis-slice that only becomes live once trailing knob parameters exist — good catch. gen_absY_max is opt-in and gen-level-only as described.

6. Runcard. The new [QED] block (qed_isr = nllp, qed_kappa = yes, mixed off/default) matches the description's recommended B′ configuration, and the updated comment's vector count is right (α_s + 4 eff NP + 4 γν NP + 10 TNP + 3 κ = 22).

Nits (non-blocking): self._fixed_ratio_path is assigned twice in __init__; self._qed_knob_logT is set but never read (the compute path iterates self._qed_knobs directly).

CI / validation products

  • Workflow run 43301046492 on df1cdee: all 25 jobs green.
  • PRValidation/PR770/2026_11_12 vs the latest scheduled main reference ReferenceRuns/2026_11_12_e0298952d (scheduled column currently green): 116 of 122 number logs identical after stripping run-identity lines (paths/timestamps/git hash). The 6 residuals are single-cell last-digit jitter in postfit table dumps — relative ~1e-6–4e-6 (e.g. BSM postfit-η inclusive 129652.264 vs ...265; unfolding_mw data sum-of-weights ±3e-5) — i.e. fit-convergence-level noise between independent runs. All alpha_s genfit / reco-2d / reco-4d pull-and-impact results are identical, and spot-checked postfit PNGs are byte-identical. Nothing in main.yml exercises the new scetlib_ad paths (the AD param model is not run by CI; the theoryCorr list the alphas-z jobs use is the multi-entry one for which old and new resolution coincide), so "identical within convergence jitter" is exactly the expected-and-understood outcome.

hodor

The hodor review check was still running when this was posted, so there was nothing of its to confirm or correct yet; the static analysis above stands on its own. (If its verdict lands with anything contradictory, I'll follow up in-thread.)

Verdicts

  • qedStep_* tape nuisances + selection/refusals: expected-and-understood (code-level; the physics response comes from the κ-cache the author validated offline: tape-vs-frozen Asimov and data-fit comparisons quoted in the description).
  • [QED] check + escapes: expected-and-understood — closes a genuine silent-mismatch hole.
  • theory-helper separator/sidecar fixes: expected-and-understood; validated against actual histmaker emission and syst naming.
  • Envelope mis-slice fix: an actual latent bug fixed, not a cosmetic change.
  • CI: expected-and-understood (identity within convergence jitter).

Watch next: the first production B′ κ-cache build once scetlib-cms !29 lands (PDF-dependent build, needs the matching QED-on theory correction or the new refusal fires — by design), and the first top2 fit on data: the qedOrder grouped impact against the author's quoted 0.078→0.101×10⁻³ NNLL→B′ growth.

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issues Found

Minor (P3)

  • [P3] Align qed_nuisances disable spellings with qed_select_steps (/home/c/cmsmwbot/WRemnantsCI/actions-runner9/_work/WRemnants/WRemnants/wremnants/postprocessing/scetlib_ad/param_model.py:593-595)
    • When the cache registers no qed_kappa_* parameters, resolve_qed_steps refuses any qed_nuisances token it deems "explicit" unless it is in ("top2", "default", "", "all", "none", "off"). But params.qed_select_steps -- the function used when the cache DOES have the multipliers -- accepts ("none", "off", "0", "no", "false") as "float none". So qed_nuisances=0 (or no/false), accepted elsewhere in this feature and mirroring the ("none", "0", "off") spellings accepted by fixed_ratio/qed_knob, raises "qed_nuisances='0' names QED order steps, but this cache registers no qed_kappa" on a non-kappa cache, while the identical token on a kappa cache means "none". The same branch also silently treats qed_nuisances=all as an inactive no-op on such caches even though 'all' explicitly asks for steps. Extend the exclusion set to match qed_select_steps.
  • [P3] EW-ISR double-count guard does not fire for qedTheta knobs (/home/c/cmsmwbot/WRemnantsCI/actions-runner9/_work/WRemnants/WRemnants/wremnants/postprocessing/scetlib_ad/param_model.py:127-133)
    • The new CONFLICTS entry refuses cards carrying ew.isr systs (pythiaew_ISR, setupRabbit --isrUnc) only when a qedStep name is fitted. The same physics also enters through qed_knob: with qed_nuisances=none plus qed_knob, the fitted nuisance is qedTheta/qedTheta2..., no qedStep* is in _param_order, the lambda never fires, and a card built with --isrUnc would apply the Pythia ISR template AND the frozen QED template T**theta with no refusal -- the combination the model itself calls "counts the QED order uncertainty twice" when policing qed_knob vs on-tape steps. This matters for new knob fits on --isrUnc cards (legacy replays that intentionally kept those systs would need --excludeNuisances). Consider having the lambda also match the knob names ("qedTheta*"), or documenting that knob users must drop the card's ISR systs.

Summary

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

Overall Verdict

Status: Patch has blocking issues

Explanation: The on-tape QED order-step machinery (step selection, registration, unit-nuisance reparam theta = kappa - 1 with anchor 1 and N(0,1) prior, envelope-slice fix, and the frozen-template/fixed-ratio/gen_absY_max hooks) is internally consistent: parameter ordering, TF slices for envelope/knobs, the K-less anchor denominator, and the cache-vs-correction [QED] effective-config comparison all check out, and the rabbit_theory_helper sidecar resolution is safer than the heuristic it replaces. The two findings are minor: an inconsistent set of accepted "none" spellings for qed_nuisances on caches without qed_kappa, and the card double-counting guard for EW ISR templates not extending to the qedTheta frozen-template knobs.

Review details
  • Model: cern/Kimi-K3-FLATIRON

Review metrics: 42 turns · 57 tool calls · 13m 13s

  • Tokens: 2.69M input · 2.58M cached (96%) · 49.3K output

…d_knob

Review of #770 (hodor P3 x2, cmsmwbot nits):
- params.qed_rule_kind parses the qed_nuisances spellings in one place.
  On a cache without qed_kappa the default and every 'none' spelling
  (none/off/0/no/false) are now accepted, and 'all' is refused like an
  explicit list, instead of 0/no/false being refused and 'all' silently
  ignored.
- A card carrying EW ISR systs (ew.*_isr, pythiaew_ISR) next to qed_knob
  templates now WARNS of the double count. It does not refuse: past knob
  fits ran on cards that keep those systs and freeze them with
  --freezeParameters, which the model cannot see. The refusal for floated
  qedStep_* is unchanged.
- Drop the duplicate _fixed_ratio_path assignment and the unused
  _qed_knob_logT.

Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01Sr6N7Ywi7iExTtmJDiju9W
@lucalavezzo

Copy link
Copy Markdown
Collaborator Author

Addressed in 9db9b87.

hodor [P3] qed_nuisances disable spellings. Fixed. A new helper, params.qed_rule_kind, is now the only place that parses qed_nuisances, and both qed_select_steps and _resolve_qed_steps use it. On a cache built without qed_kappa, the default (top2) and every none spelling (none/off/0/no/false) are now inert. all is refused like an explicit list, so it is no longer a silent no-op.

hodor [P3] EW ISR guard vs qed_knob. This is now a warning, not a refusal, and the choice is deliberate. Earlier qed_knob fits (the frozen-template QED studies) ran on cards that keep pythiaew_ISR_Corr but freeze it with --freezeParameters. The param model cannot see that freeze, so a refusal would break that workflow. A card with ew.*_isr systs next to qedTheta* now prints a WARNING naming the systs and both remedies (freeze or --excludeNuisances). The refusal for floated qedStep_* is unchanged. Both behaviours are in the qed_nuisances docstring.

cmsmwbot nits. I removed the duplicate _fixed_ratio_path assignment and the unused _qed_knob_logT. I left the leftover _corr_sep == "_" fallback in add_pdf_alphas_variation alone; it is harmless, as noted.

Tests. The mock-core unit suite now has 98 checks, all passing: the earlier 77 plus 21 covering the parser and every spelling on κ and non-κ caches, all refused on a non-κ cache, the knob + EW ISR warning, the qedStep + EW ISR refusal still in force, and no warning when neither is present. black, isort and flake8 are clean on the changed lines (container, CI flags).

🤖 Generated with Claude Code

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issues Found

No issues found.

Summary

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

Overall Verdict

Status: Patch is correct

Explanation: The delta unifies qed_nuisances parsing in qed_rule_kind with identical behavior in qed_select_steps and the corrected inert/refused split (0/no/false inert, all refused) on non-kappa caches; the new qed_knob + EW-ISR warning in _check_double_counting runs after _knob_names is initialized, matches case-insensitively like the _CONFLICTS enforcement, and has a well-formed f-string; removals of the duplicate _fixed_ratio_path assignment and unused _qed_knob_logT leave no dangling references. No qualifying findings.

Review details
  • Model: cern/Kimi-K3-FLATIRON

Review metrics: 9 turns · 12 tool calls · 2m 3s

  • Tokens: 142.9K input · 123.3K cached (86%) · 8.8K output

@cmsmwbot

cmsmwbot Bot commented Nov 12, 2026

Copy link
Copy Markdown

Ack for 9db9b872 —

On the cmsmwbot nits from my review: verified in the df1cdee…9db9b872 diff. The duplicate self._fixed_ratio_path assignment is gone, and both _qed_knob_logT write sites (the None init and the _qed_knobs[0][1] set) are removed with no dangling references — hodor's re-review of 9db9b872 independently confirms this. The _corr_sep == "_" fallback in add_pdf_alphas_variation staying is the agreed call (dead but harmless).

On hodor's P3s: additive to my earlier review, nothing contradictory, so nothing to correct. The resolutions look right: params.qed_rule_kind as the single parser with the none-spellings inert / all refused split on non-κ caches matches the contract I validated, and demoting the qed_knob + EW-ISR guard to a named WARNING preserves the legacy frozen-template (--freezeParameters) workflow that a refusal would have broken — the refusal for floated qedStep_* stays, which is the case that matters.

CI is re-running on the new head; consolidated status will follow once the run finishes.

@cmsmwbot

cmsmwbot Bot commented Nov 12, 2026

Copy link
Copy Markdown

✅ CI green on 9db9b87 — all 25 jobs passed (run 43318539861, wall ~1h24m). Slowest: w-analysis 1h12m, then lowpu-w 52m, alphas-z-histmaker 50m. Ready for review.

@davidwalter2 davidwalter2 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The independent QED order-step nuisance treatment makes sense to me, and I support keeping QED enabled in the Z runcard. A few points on the current head, 9db9b872:

  1. Unit tests. The 98 mock-core checks sound useful, but they are not included in the PR, and the existing CI does not exercise this AD path. WRemnants does not currently run a unit-test suite, so this does not all need to be set up here. Could you either include the tests and set up their execution in this PR, do that in a follow-up PR, or open an issue to track it?

  2. Description of the lower-order endpoints. Please clarify the statement that theta = -1 gives “the next-lower order exactly.” Dropping the highest active step does that, but at NLL' central, setting only qedStep_nll=-1 leaves the NLL' constants and fixed-order contribution active. Reaching LL requires both qedStep_nll and qedStep_nllp to be -1. The independent variation of the increments makes sense; this is a request to improve the description, not to change the nuisance treatment.

  3. Boolean spelling bug. In params.py, _QED_BOOL normalizes yes/true/1 and no/false/0, but omits on/off, which SCETlib accepts through ConfigParser.getboolean. Consequently, physically identical settings such as mixed_running=on versus yes, or qcd_side_mixed=off versus no, are rejected by the new cache/correction consistency check. Please add those two aliases.

  4. Matching cache and central correction. With NLL' QED enabled in the Z runcard, could you include the matching cache and QED-on central theory correction as part of this PR, through the usual artifact/data mechanism, and document which files go with this configuration? If they are supplied through a separate data update, please link it here. Keeping the option enabled is the right direction; the corresponding inputs should be available together so the new configuration can be used consistently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants