Add hist rebin limit plots - #20
Conversation
|
@cms-flaf-bot please test
|
|
pipeline#15502299 started |
There was a problem hiding this comment.
Pull request overview
This PR replaces the old flat HistMerger → CreateDatacards → ResonantLimits flow with a new chain that rebins the 2D (DNN × HME) merged histograms into significance-optimized 1D shapes, builds datacards (including meta-era combinations that sum sub-eras), computes resonant limits, and draws overlay limit plots. It also adds stacked plots of the rebinned shapes and utilities for external/projected limit curves, alongside broad PEP8-style reformatting of the dc_make modules.
Changes:
- New law task chain (
MergedHists,HistRebinTask,RebinningParamsTask,PlotResonantLimitsTask) with binning parameters hashed into output paths and meta-era support. - New scripts:
hist_rebin_2d.py(significance-driven 2D→1D rebinning),plot_rebinned.py,lumi_projected_limits.py,hepdata_to_external_limits.py. maker.pygains meta-era combined-shape handling and revised negative-bin resolution;common/tools.pyadds tolerated negative-integral clamping.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| law/tasks.py | Rewrites the task graph around rebinning, datacards, limits, and new limit-plot tasks |
| dc_make/hist_rebin_2d.py | New 2D→1D significance-sliced rebinning script |
| dc_make/maker.py | Adds meta-era combined shapes, per-category datacards, refactored negative-bin handling |
| dc_make/plot_rebinned.py | New stacked plots of rebinned per-slice shapes |
| dc_make/lumi_projected_limits.py | New luminosity-projection external-limits JSON generator |
| dc_make/hepdata_to_external_limits.py | New HEPData→dhi external-limits converter |
| dc_make/uncertainty.py | Reformatting plus finer-grained getUncertaintyForProcess matching |
| dc_make/process.py | Reformatting plus allowsNegativeBinsWithinError helper |
| common/tools.py | Reformatting plus tolerated negative-integral bin clamping |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
pipeline#15502299 failed |
|
@cms-flaf-bot please test
|
|
pipeline#15504166 started |
|
@cms-flaf-bot please test
|
|
pipeline#15504367 started |
|
pipeline#15504367 passed |
|
pipeline#15504166 failed |
|
@cms-flaf-bot please test
|
|
pipeline#15509624 started |
|
pipeline#15509624 failed |
|
@cms-flaf-bot please test
|
|
pipeline#15509944 started |
|
pipeline#15509944 passed |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Suppressed comments (1)
dc_make/maker.py:916
- Same defect as in the main loop:
unc.typeis accessed on the raw uncertainty, butAutoUncertaintyhas notypeproperty, so a meta-era combined with anauto-type uncertainty raisesAttributeErrorhere in thekeep_all_signal_hypothesis_into_single_datacardpath. Gate on the concrete lnN classes instead.
if self.isMetaEra(era) and unc.type == UncertaintyType.lnN:
|
@cms-flaf-bot please test
|
|
pipeline#15512406 started |
|
pipeline#15512406 passed |
kandrosov
left a comment
kandrosov
left a comment
There was a problem hiding this comment.
Hard to follow. Split PR into two: one that applies formatting without touching logic, other that changes logics.
1e5b993 to
71fb60d
Compare
addUncertainty walks the process/era/channel/category product and adds nothing where the
uncertainty does not apply, so an entry that matches nothing at all is not an error
anywhere: it simply never appears and the card is quietly short a nuisance. Three BR
uncertainties were lost that way and only turned up by counting the card against the
configuration.
The cause there is worth naming, since the configuration invites it: a signal's ${MX} is
already resolved to the mass being built by the time an uncertainty is matched, so a
`processes:` pattern written as "XtoHHto2B2W_2L_${MX}" matches the literal string and
nothing else. Uncertainty.hasMatch takes a ^-anchored pattern as a regex, which is how
such a list has to be written.
Warns rather than raises: an era-scoped nuisance legitimately reaches nothing in a card
built for a different era.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
Two statements were over the line length black keeps: the mc_stats guard's condition and the dict comprehension filtering the spec down to MergePullsAndImpacts' parameters. No behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
|
@cms-flaf-bot please test
|
|
pipeline#15644110 started |
The block only ever reached the combined card, so there was no way to ask for the
impacts of one category -- which is most of what the ranking is wanted for, since a
nuisance that dominates res2b need not dominate boosted.
An entry may now carry a `glob:`, resolved against the era's datacard directory exactly
as limit_plots resolves its own, with ${ERA} substituted. The glob and the mass together
must select exactly one card: PullsAndImpacts fits a single workspace, one combine job
per parameter, so a set is not something it can be handed. Failing that, the error names
the directory it looked in and lists what is actually there, since the usual causes are
a mass that was never built and a mistyped category.
An entry with no `glob:` keeps the old behaviour and uses the combined card, which
already contains every era. Entries that do carry one are drawn per era, since that is
the level their cards live at, and the output is laid out <name>/<era>/<mass>/ so two
entries cannot collide.
Verified against real cards: res2b at 300 and 600 and boosted at 800 each resolve to
their own card and their own dhi output path, a mass that was not built and a mistyped
category both raise with the directory listing, and an entry without a glob still finds
combined_500.txt.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
The impacts are worth running on HTCondor -- the fits are one combine job per nuisance per direction, which is where all the time goes and which parallelises perfectly. But the knob is on PullsAndImpacts, the fit workflow, not on PlotPullsAndImpacts, which is not a workflow at all and has no `workflow` parameter. law namespaces an upstream task's arguments as --<TaskName>-<param>, so they can never be parameters of the task being run and cannot go in the spec, which validate_plot_params checks against exactly that. So an entry may carry `dhi_args:`, appended to the command verbatim: dhi_args: [ --PullsAndImpacts-workflow, htcondor ] Verified that law takes them: with that argument the graph resolves PullsAndImpacts(effective_workflow=htcondor, workflow=htcondor) while CreateWorkspace stays local. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
dhi generates the Asimov at r=1 unless told otherwise, and r=1 is not a meaningful
reference anywhere on this scan. The limits on these cards run from ~6.8 at 300 GeV to
~0.065 at 800, so r=1 is an almost invisible signal at one end -- the ranking is then
really the background-only one -- and ten times more signal than can be excluded at the
other, which pins r and collapses every impact towards zero. That is visible in the
robust run already done: b-tagging impacts of 0.9 at 300 against 0.003 at 800.
An entry may now set `poi_value:`, either a number or "limit", which takes the expected
limit for that mass from the limits already merged for the same family of cards. It is
looked up rather than recomputed -- the family is the era glob the limit plots use, since
the combined card's own combined_*.txt family has no limits and asking for them starts a
fresh workspace and fit per mass.
It reaches combine as --expectSignal through PullsAndImpacts' custom_args. Not through
dhi's `parameter_values`, which looks like the natural home and is not: for a resonant
search hh_model is NO_STR, and POITask then returns '""' for the joined parameter values
and hard-codes the output postfix to r=1.0, so the value is dropped in silence.
Known limitation, documented in the config and the docs: custom_args is significant for
task identity but is not part of the store path, so every value of --expectSignal writes
to the same file. Changing poi_value therefore needs the PullsAndImpacts,
MergePullsAndImpacts and PlotPullsAndImpacts outputs cleared first, or law reports the
previous fit as complete.
DhiPlotMixin.plot_command also learns dhi's MultiCSVParameter pair form, ("r", 6.8) ->
r=6.8, which the parameter_values attempt needed and which the next such parameter will.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
Migrad fails on some of these fits from a bad seed gradient, and dhi then aborts the whole merge rather than the branch: at 300 it was CMS_eff_m_id_iso_2023, and 80 converged fits produced nothing. dhi offers two ways out, retrying with MINIMIZER_no_analytic or skipping the branch. Skipping drops a nuisance from the ranking, which is the same fault as the robust method silently losing the jet energy scale, and no_analytic tends to move this failure to another parameter rather than cure it. So an entry may carry `fit_args:`, joined with --expectSignal into PullsAndImpacts' custom_args, which is what reaches combine. A Simplex fallback there converges it: fit_args: [ --cminFallbackAlgo Minuit2,Simplex,0:0.1 ] Note this lands ahead of dhi's own two fallbacks rather than after them -- the command template puts custom_args near the front and combine_optimization_args near the end -- so Simplex is tried first. That suits this failure, which loosening Migrad's tolerance does not address. Verified end to end: with the fallback, CMS_eff_m_id_iso_2023 converges, all three masses complete, and every one ranks all 79 card nuisances with nothing dropped, against 72-74 under `robust`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
|
@cms-flaf-bot please test
|
|
pipeline#15646468 started |
|
@kandrosov this should be ready for real review, I want to be done with it |
kandrosov
left a comment
kandrosov
left a comment
There was a problem hiding this comment.
Reviewer note: this review was performed by Claude (Anthropic's AI, running in Claude Code). Every finding below was independently re-verified against the actual law 0.1.20, dhi, FLAF and CombineHarvester sources before being reported; the blocker was additionally reproduced by executing this PR's own code with the production binning knobs. Line numbers refer to the PR head (71b010d).
Overall assessment
This is a carefully engineered PR, and much of it verifies cleanly: the Asimov significance matches Cowan et al. (arXiv:1007.1727) exactly, data is never consulted by the binning optimization and is rebinned — along with every systematic variation — with the single frozen slice mapping, the new task outputs are atomic via localize("w"), ROOT object ownership is handled correctly throughout, in-process dhi task construction goes through modify_param_values so path lookups agree with the CLI runs, and the comments document real non-obvious rationale. The integration pipeline is green at this commit.
That said, the review found 1 blocker and 5 major issues, plus a set of minors. They cluster in exactly the places green CI cannot see (see the CI-coverage note at the end): the degenerate-slice path of the rebinner, and the era_groups/meta-era plumbing, which is currently only half-wired.
Severity scale: blocker = wrong physics results on a mainline path, must be fixed before merge; major = wrong behavior on a realistic shipped path; minor = worth fixing, limited blast radius; nit = optional polish.
Blocker
B1. bin_opt_2d/rebin_2d.py:461 — degenerate slices silently duplicate the full 2D plane into extra datacard categories
find_slices must emit exactly n_slices slices, so when the axis is exhausted early — grow_slice's documented fallback (return best_left if best_left is not None else first_bin, line 427) takes the whole remaining axis whenever no window clears the background floors — the filler branch at lines 461–464 emits slices (0,0), (-1,-1), (-2,-2). After slices.reverse(), extend_outer_edges (line 605) rewrites the first one to an inverted range like (0,-1). ROOT silently reinterprets any inverted bin range as the full axis including under/overflow (verified empirically: TH2::Integral(-1,-1,…) = Integral(0,-1,…) = full plane; same for IntegralAndError at line 837 and ProjectionY).
Reproduced end-to-end with this PR's code and the production knobs from binning_2d.yaml (n_slices=4, min_slice_bkg_sum=1.0, min_signal=0.5): a category with signal 0.6 and background 0.8 produced slices [(0,-2), (-1,-1), (0,0), (1,nx+1)], and the rebinned output held the entire plane's content three times (summed signal 1.8 vs true 0.6; background 2.4 vs 0.8). Every process, every systematic and data_obs are duplicated consistently, all yields stay positive, so nothing downstream — resolveNegativeBins, the datacard maker, combine — can detect it: the same events enter the fit as 2–3 statistically independent categories, overconstraining the limit in exactly the sparse high-mass categories that drive sensitivity. CI sets n_slices=1 (CI_binning_2d.yaml), which takes only the last-slice branch, so the green pipeline proves nothing here.
Fix: represent an exhausted slice explicitly (e.g. x_range=None) and have rebin_hist_2d write an all-zero histogram for it instead of calling IntegralAndError; additionally assert 0 <= lo <= hi in rebin_hist_2d/slices_to_record so an invalid range can never reach ROOT's silent full-range reinterpretation, and never let extend_outer_edges assign full_lo to a slice whose hi < full_lo.
Major
M1. dc_make/maker.py:1006 — multi-value lnN silently dropped for subprocess-merged processes
The old getMultiValueLnUnc had a yield-weighted fallback for processes with subprocesses; the refactor deleted it, and the only remaining lookup is unc.getUncertaintyForProcess(process.name, era, channel, category) (also at line 1104), which has no subprocess awareness. Both shipped configs are hit: the new bbWW TotalBkg (subprocesses TT/DY/ST/VV, SR/boosted) loses PDF_alphas and top_mass (~2.5–2.8 % each) across the entire boosted region with no message, and the existing bbtautau VH/pdf_Higgs case regresses from a yield-weighted lnN to nothing. Plain lnN still matches through appliesTo's subprocess check, so plain and multi-value lnN now behave oppositely — and the meta-era path (_getSubEraLnNVariedShapes) does still resolve per subprocess, so the plain-era and meta-era paths disagree about the same config entry. warnUnusedUncertainties cannot catch it because the uncertainty is still used on unmerged processes elsewhere.
Fix: reinstate subprocess handling on the regular-era path (the meta-era code already applies per-subprocess values and can share _getLnNValue), or at minimum warn loudly when a MV lnN matches a merged process's subprocess but not the process.
M2. law/ResonantLimitsTask.py:77 — limits.npz double-counts data when eras lists a group and its members
run() collects datacards over get_eras() (= get_all_eras(), every era) into a single MergeResonantLimits. dhi groups cards purely by the trailing mass number and runs combineCards.py over all cards of a mass, so each per-mass limit is a joint fit of every listed era's card together. The shipped x_hh_bbww_DL_run3_1D.yaml (HH_bbWW #107) lists the four sub-eras and Run3_Early — so on that documented path each per-mass workspace combines all five cards and every 2022–2023BPix event is counted twice; limits.npz silently holds too-strong limits. The get_top_level_eras() guard at lines 98–104 protects only the combined_<mass>.txt cards, and the comment "Every era still gets its own limit above" is not true of the npz (one row per mass, not per era). The mainline 2D config (eras: [Run3_Early] only) is unaffected, which is also why CI (four disjoint eras) cannot see it.
Fix: build the yielded MergeResonantLimits from get_top_level_eras() cards only, mirroring the combined-card loop, and fix the comment.
M3. law/CreateDatacardsTask.py:24 (and PreprocessShapesTask.py:37) — meta-era and member-era instances collide on law's HTCondor control files
meta_era is significant (two distinct task ids) but store_parts() stays (version, class, period), and the meta instance is created with period = era_groups[era][0]. FLAF's htcondor_output_directory() is LocalDirectoryTarget(self.local_path()) and the jobs file is named only by the branches repr — both instances have branch map {0: None} — so CreateDatacardsTask(period=Run3_2022) and CreateDatacardsTask(period=Run3_2022, meta_era=Run3_Early) share the same submission/status file, and remote_log_dir_target() collides too. With the shipped 1D config (group + members in eras:), ResonantLimitsTask requires both instances in one run; since HTCondor is the default workflow (NO_STR resolves to the first workflow class in the MRO), the second workflow resumes from the first one's submission data (law/workflow/remote.py:656), polls the other task's condor jobs, never submits its own, and ends incomplete. Outputs themselves do not collide (they use datacard_era). CI runs --workflow local, so it cannot see this.
Fix: include the datacard era in store_parts() for the meta-era-capable tasks, e.g. (self.version, self.__class__.__name__, self.datacard_era).
M4. law/CreateDatacardsTask.py:327 — meta-era shape plots carry one sub-era's luminosity label; plot/Run3_Early.yaml is dead config
plot_rebinned_shapes hadds all sub-era shapes into one summed file but passes --year self.period. HistPlotter.py loads config/plot/{year}.yaml for the lumi label, so the four-era Run3_Early sum is stamped "7.9804 fb⁻¹" (Run3_2022) instead of 62.4. HH_bbWW #107 ships config/plot/Run3_Early.yaml with exactly the right label — but nothing in either PR ever passes Run3_Early as --year, so that new file is never read and every mainline Run3_Early shape plot has an 8×-wrong luminosity.
Fix: pass "--year", self.datacard_era (keeping --period self.period, which must stay a real era for the Setup).
M5. bin_opt_2d/rebin_2d.py:1043 — frozen replay silently re-optimizes categories missing from the record
--binning promises "the recorded edges are applied as-is and nothing is optimised", and lookup_frozen's docstring says an absent category "is skipped again rather than quietly re-optimised" — but nothing implements that: lookup_frozen returns None for a missing category, and in process_category frozen is None is indistinguishable from "no --binning at all", so a fresh signal-aware optimization runs. The converse also holds: the min_signal/background gates run before the frozen branch, so a recorded category whose signal dips below min_signal in the new input is silently dropped from the replay even though its edges are in the record. Both directions break the reproducibility guarantee freezing exists to give (new histogram version, added systematic, unblinding).
Fix: when a frozen record is supplied, a missing category must be an explicit skip (with a printed reason), and the gates must be bypassed (or fail loudly on mismatch) so the replay reproduces exactly the recorded category set.
Minor
law layer
law/ResonantLimitsAndHistPlotTask.py:33—get_plottable_erasfilters group names out oferas:but never substitutes their members (the docstring promises the opposite). With the shipped 2D config (eras: [Run3_Early]only) it returns[]: the task requires zeroHistPlotTasks, touchesdummy.txtand reports success with no histogram plots at all. Fix:plottable += era_groups.get(e, [e]), deduplicated.law/ResonantLimitsAndHistPlotTask.py:38—HistPlotTask.req(self, period=e)forwards--version, not--hists-version; with the documented--version <own> --hists-version <central>pattern this schedules the entire histogram production chain under the new version instead of plotting the existing central histograms. Fix: forwardversion=self.input_hists_version.law/PlotPullsAndImpactsTask.py:305—--expectSignal/fit_argsreach combine only through dhi'scustom_args, which is not encoded in any dhi output path (dhi/tasks/base.py:330says so explicitly; nothing inget_output_postfix/store_partsuses it). Changingpoi_value/fit_args(or the expected limit moving after the cards change) and re-running under the same version silently reuses the stale fits —--redrawre-draws from those stale fits. The comment at lines 176–179 asserts the opposite and is factually wrong; the config-side WARNING about hand-deletinginference/data/storeis the only guard. Fix the comment, and ideally derive the dhiversionfrom a hash of the custom args so changed fit settings move the store path.law/ResonantLimitsTask.py:54— the per-era datacard mirror written bystage_datacardsis an undeclared output thatPlotResonantLimitsTask/PlotPullsAndImpactsTaskglob: ifCreateDatacardsTaskre-runs whileResonantLimitsTaskstays complete, downstream plots silently use the old cards; if the mirror is deleted, nothing in the graph can regenerate it.law/ResonantLimitsTask.py:108— non-atomic outputs:limits.npzis copied and thecombined/dirtouch()ed before the per-masscombineCards.pyloop; a failure mid-loop leaves both declared outputs existing, so the next run reports the task complete with missing/partial combined cards (and stale masses in the dir are never cleared). Every other task in this PR moved to atomiclocalize("w")— this one should too.law/PreprocessShapesTask.py:54(andCreateDatacardsTask.py:61) —workflow_requires()does not mergesuper().workflow_requires(), dropping FLAF's HTCondor bundle requirements. Fix:reqs = super().workflow_requires(); reqs.update(self.input_hist_reqs()).law/PlotResonantLimitsTask.py:277— all entries of an era share one output directory, and the plot basename is keyed by the entryname(default"limits"): two entries without distinct names silently overwrite each other's plots. Validate uniqueness or use per-entry subdirectories asPlotPullsAndImpactsTaskdoes.law/DhiPlotMixin.py:75— aFalsebool inplot_paramsis silently dropped from the command (bools are set by presence), whileplot_targetsconstructs the task with theFalsevalue — a default-True dhi bool can therefore never be unset, and path lookup and command can disagree.law/StatInferenceTask.py:140—StatInference.param_valuesrestricts only the datacard step;get_masses()/get_required_variables()still require and stage every mass in the config, so the inputs/preprocess do not shrink accordingly.
dc_make
dc_make/uncertainty.py:299— an emptyprocessestuple in a MV lnN sub-entry flips from never-match (old code) to match-all with first-match-wins. Latent today (the one affected config, bbtautauQCD_norm, is masked by its ownRun_2018vsRun2_2018era mismatch), but it will fire the day those era names are fixed, applying a QCD-only normalization to every process. Require non-emptyprocessesinfromConfig, or scope the bbtautau config explicitly.dc_make/maker.py:389— the meta-era lnN→shape conversion builds Up/Down from raw sub-era sums while the nominal is the negative-bin-clamped combined shape, so the templates disagree with the nominal in every clamped bin (nominal 0, variations negative). Latent only because no shipped lnN is era-scoped; the first per-era lumi split — the reason this machinery exists — triggers it silently. Run the summed variations through the same validation as the nominal.dc_make/maker.py:535— theif sub_hist is None: continuetolerance ingetCombinedShapeis dead code (_rawShapealways raises for a missing histogram), whilehasNominalShapeadmits a process if any sub-era has the shape — so partial availability across sub-eras crashes the card build. Decide the contract and make the two halves agree.dc_make/maker.py:982—getSignalProcessForParamsreturns the first matching signal, andaddUncertaintyskips the bin's background uncertainties when that one has no shape — butaddProcessadds the backgrounds if any signal with those params has a shape. A bin where only the second signal exists gets backgrounds with zero systematics, silently. MirroraddProcess's any() logic.dc_make/maker.py:1088— thekeep_all_signal_hypothesisblock converts every lnN to a shape template for meta-eras (missing thelnNIsEraDependentgate the primary block has) and also lacks itsprocessInBingating; the two near-identical loops should be factored so they cannot drift.dc_make/maker.py:1244— nothing rejects a config whoseeras:lists a meta-era together with its sub-eras; the standalonecreate_datacards.pyentry point (no--eras) then writes a combined card that double-counts. Raise inDatacardMaker.__init__.dc_make/maker.py:507— the new meta-era asimov branch copies the context-free"Cannot create asimov data histogram"message; add era/channel/category.
rebin_2d / tools
bin_opt_2d/rebin_2d.py:987— the replay ignorescategory_patternandslice_varstored inbinning.json(stored precisely to make the record self-describing) and always takes them from the current knobs; a standalone replay without the matching--binning-configsilently writes empty shapes.bin_opt_2d/rebin_2d.py:987— nothing enforces that the binning yaml'scategory_patternmatches the datacard config's, despite both HH_bbWW configs carrying "Must match" comments andrebin_2dalready parsing the datacard config; a mismatch does not raise (each slice just becomes its own base category). Compare and raise inrun().bin_opt_2d/rebin_2d.py:276—significance_modefrom the binning yaml is never value-checked (the CLI haschoices=, the yaml path does not); any typo silently falls back to"sb".bin_opt_2d/rebin_2d.py:1069—run()exits 0 even when every category was skipped and zero shapes were produced; raise when an era's binning record ends up empty.common/tools.py:87—CategoryNaming.expand()has no caller anywhere (docstring claims consumers); drop it or fix the docstring.
docs (companion HH_bbWW #107) and comments
docs/stat_inference.md:229— the docs statemethod: robustis "what this analysis uses"; the shipped config deliberately usesmethod: default.docs/stat_inference.md:99— the standalone rebin example uses$ANALYSIS_BIG_DATA_PATH, which no current env script exports.README.md:7— "Neither is part of the datacard chain" contradicts thePreprocessShapesTaskwiring this same PR adds (the chain schedules the rebinning as a dependency); the true invariant is "the datacard step itself does no binning".bin_opt_2d/rebin_2d.py:32— comment points readers atbin_opt_2d/binning.yaml, which does not exist (the real file is the analysis'sconfig/Datacards/binning_2d.yaml).law/PreprocessShapesTask.py:103— comment says "relative paths are resolved against the analysis area", but only args starting withconfig/are; narrow the comment or generalize the code.
Nits
dc_make/process.py:59 appliesToCategory prefix matching can over-match a sibling base category (SR/boosted also matches SR/boosted2) and diverges from the uncertainty-level categories semantics · dc_make/process.py:20 new categories=[] copies the mutable-default-argument pattern · bin_opt_2d/rebin_2d.py:871 comment references a nonexistent sum_over_sources() · bin_opt_2d/rebin_2d.py:1076 argparse description calls the script "not part of the datacard chain" and points at a --hists-version route that does not resolve · bin_opt_2d/rebin_2d.py:718 refactor-diary comments narrate the pre-PR state · bin_opt_2d/rebin_2d.py:909 messages hardcode MX= in an otherwise parameter-agnostic script · docs/stat_inference.md:163 impact-plot output path documented as ImpactPlots/<mass>/ but the code writes ImpactPlots/<name>/<era>/<mass>/ · dc_make/maker.py:165 mergedAwayIn docstring says MinorBkg, the shipped config says TotalBkg · law/DhiPlotMixin.py:9 first-person voice in new docstrings · law/CreateDatacardsTask.py:17 the two central tasks of the chain are the only new task classes without a docstring · law/PlotResonantLimitsTask.py:41 the dummy workflow parameter is replicated unexplained into three task classes (a one-line comment on why it exists would help).
What green CI does — and does not — prove
The HH_bbWW multi-era CI job runs ResonantLimitsAndHistPlotTask with CI_card.yaml (four standalone eras, one mass, one bin, n_slices=1, gates off), so it genuinely exercises: the MergedHists wiring, PreprocessShapesTask+rebin_2d on the standalone-era path, CreateDatacardsTask including the HistPlotter/pypdf shape plots, and ResonantLimitsTask through dhi's workspace/limit/merge plus combineCards.
CI does not touch: any era_groups/meta-era code (getCombinedShape, meta lnN-as-shape, the PreprocessShapesTask fan-out, Run3_Early), PlotResonantLimitsTask entirely (limit plots, band plots, external limits, lumi projections, campaign validation), PlotPullsAndImpactsTask entirely (impacts, poi_value: limit, fit_args), TotalBkg subprocess merging, multi-mass param-dependent backgrounds, n_slices>1, or the frozen-replay path. Before merge, one real meta-era run (a small Run3_Early version through CreateDatacardsTask → ResonantLimitsTask → PlotResonantLimitsTask) would exercise most of the major findings above at once.
Two further candidate findings were raised and then refuted during adversarial verification; details available on request.
Multi-value lnN silently skipped a merged process. getUncertaintyForProcess looks the value up by process name only, so TotalBkg -- whose values are written for its subprocesses TT/DY/ST/VV -- matched nothing and PDF_alphas and top_mass disappeared from every SR/boosted bin without a word. Plain lnN and shape uncertainties reach the same process through appliesTo, which does check subprocesses, so the two kinds behaved oppositely on the same config entry. getMVLnNValue resolves the subprocesses and yield-weights their values over the full merged yield, which is what the meta-era path already produces by scaling each subprocess's own histogram and summing. Meta-era shape plots carried a sub-era's luminosity. HistPlotter reads config/plot/<year>.yaml for the label, and --year was self.period, so the four-era Run3_Early sum was stamped 7.9804 fb^-1 instead of 62.4081. --period stays a real era; it builds the Setup, which has no meta-era names. ResonantLimitsAndHistPlotTask required no histogram plots at all when the configuration's eras: named only a group -- the mainline 2D one does. get_plottable_eras filtered the group out and never put its members back, so the task touched dummy.txt and reported success. It now substitutes the members, and forwards hists_version rather than version, so plotting existing central histograms does not schedule a new production of them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
Correctness, in rough order of blast radius: - ResonantLimitsTask built its outputs before the combineCards loop, so a failure part way through left a complete-looking task with a partial combined/ directory and stale cards for masses no longer configured. Staged and published at the end. - PreprocessShapesTask and CreateDatacardsTask replaced workflow_requires() instead of extending it, dropping FLAF's software bundles from submitted jobs. - The keep_all_signal_hypothesis loop converted every lnN to a shape for a meta-era, missing the lnNIsEraDependent gate its twin has, and collected uncertainties for processes merged away in the bin. - Meta-era lnN-as-shape templates were raw sub-era sums against a clamped nominal; each variation now goes through the same validation as the nominal. - addUncertainty gated a param-dependent background on the first signal at that parameter point, addProcess on any of them, so a bin where only the second exists got backgrounds with no systematics. - appliesToCategory matched bare prefixes, so "SR/boosted" also claimed "SR/boosted2". - rebin_2d took the category pattern and slice variable from the current knobs on replay, though binning.json records both; it now replays from the record, checks the datacard configuration declares the same pattern, validates significance_mode on the yaml path (an unrecognised value silently meant "sb"), and raises rather than exiting 0 when every category was skipped. - DatacardMaker rejects an eras: list holding a group era and its own members. - Process copies its channels/categories defaults instead of sharing the literals. Comments and messages: the custom_args note in PlotPullsAndImpactsTask claimed the opposite of the truth -- it is not in any dhi output path, so a changed fit setting silently reuses the previous fits; the asimov failures name their bin; limit_plots entries must have distinct names, since the plot file is named after the entry; the unused CategoryNaming.expand() is gone; and several comments pointed at things that do not exist (bin_opt_2d/binning.yaml, sum_over_sources(), MinorBkg). Verified: datacards for Run3_Early at MX=500 and MX=1000 are byte-identical to the previous commit's, and rebin_2d over a full Run3_2022 reproduces the pre-change binning exactly. Not taken: the review asks for `processes:` to be required in a multi-value lnN, but an empty list meaning "every process" is load-bearing -- lumi_1_13p6TeV is scoped by era alone, and requiring the field drops it from every bin. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
law sets luigi.BoolParameter.parsing to EXPLICIT_PARSING globally when it is imported (law/parameter.py:31), which removes argparse's store_true and leaves the argument at nargs="?". A bare "--y-log" therefore parses to None rather than True, luigi falls back to the parameter's own default, and the setting is silently lost -- while plot_targets constructs the task with the value, so the path a plot is looked up under is not the path it was drawn to. The shipped limit_plots style sets y_log: true, so this was live: the plots came out on a linear axis. Verified against luigi 3.7.3 and law 0.1.20 as installed here, reproducing luigi's own _parser_kwargs: "--y-log" gives None, "--y-log True" gives True. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
find_slices must return exactly n_slices ranges, so every mass point shares one category list. When the significance scan reached first_bin with slices still to place it invented the rest -- (0,0), (-1,-1), (-2,-2) -- and after the reverse and extend_outer_edges the first became an inverted range like (0,-1). ROOT reads any range whose upper bound is below its lower one as the whole axis including under/overflow, so that slice became a copy of the entire 2D plane: the same events entered the fit again as an independent category, consistently across every process, every systematic and data_obs, with all yields positive and nothing downstream able to tell. It fires. Rebinning Run3_2022 from v2605a with the shipped binning_2d.yaml knobs, four categories came out duplicating their plane and a fifth carried an empty underflow-only slice: MX=300 eMu SR/recovery [(0,0), (1,1), (2,2), (3,51)] empty category MX=500 eMu SR/boosted [(0,-1), (0,0), (1,25), (26,51)] full plane again MX=550 eE SR/boosted [(0,-1), (0,0), (1,25), (26,51)] full plane again MX=550 eMu SR/boosted [(0,-1), (0,0), (1,25), (26,51)] full plane again MX=550 muMu SR/boosted [(0,-1), (0,0), (1,25), (26,51)] full plane again An exhausted slice is now None rather than an invented range, discover_binning returns None if any slice is one, and process_category skips the category with a reason -- the same way it already skips one with too little signal. rebin_hist_2d additionally refuses a range that is not 0 <= lo <= hi, so an invalid one can never reach ROOT's silent reinterpretation again. Verified over a full Run3_2022: 4 degenerate slices before, 0 after; exactly those five categories dropped, and all 77 surviving categories keep byte-identical edges. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
M2, ResonantLimitsTask: one merge per era, never one over every era's cards.
dhi groups the datacards it is handed by the mass in their file name alone and
runs combineCards over each group, so a single MergeResonantLimits given five
eras' cards fits all five together per mass. x_hh_bbww_DL_run3_1D.yaml lists the
four sub-eras and Run3_Early, which is their combination, so every 2022-2023BPix
event was counted twice and limits.npz held one over-strong limit per mass rather
than the per-era limits get_all_eras() describes. Each era now gets its own merge
in era_limits/limits_<era>.npz, and limits.npz is the combination: the single
top-level era's own, or a merge of the cross-era combined cards when a
configuration lists several. Unchanged for eras: [Run3_Early]; the CI's four
standalone eras keep the same joint fit, now routed through the combined cards.
M3, store_parts: keyed on datacard_era rather than period, and datacard_era moved
to StatInferenceTask so every task has it (equal to period unless meta_era is
set). A meta-era instance carries period = one of its sub-eras, since FLAF's Setup
only knows real periods, so the two collided in law's HTCondor control files --
htcondor_output_directory() is local_path(), and both have branch map {0: None}.
A run needing both had the second workflow resume from the first's submission
data and finish incomplete. Declared outputs already keyed on datacard_era and do
not move.
M5, rebin_2d: --binning now means what it says. A category absent from the record
is skipped with a reason instead of being quietly re-optimised, and the
min_signal/background gates are not applied during a replay -- re-running them
against a new input can only disagree with the record, dropping a category whose
edges are written down because its signal has since dipped. Neither showed up in
a same-input replay, where the gates happened to drop the same categories twice.
Verified: replaying a full record reproduces it exactly; a category deleted from
a record is skipped, not re-derived; a non-replay run over Run3_2022 is unchanged;
and the merge plan is unchanged for the 2D config, five merges instead of one for
the 1D config, and the same joint fit for the CI's four eras.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
|
@kandrosov I see your claude review and raise my own (lxplus doesn't have gh so I had to copy and paste this 😢 ) Thanks — this was a careful review and most of it landed. Everything below is BlockerB1, degenerate slices. Fixed in What decides it is axis granularity, not the era. That run used a 50-bin DNN axis; An exhausted slice is now MajorsM1, multi-value lnN on merged processes. Fixed. New For the record, the effect on results is nil. Rebuilding The 12 merged bins hold 438 background events of 585,472 in the card, and inside M2, M3, HTCondor control-file collision. Fixed. M4, meta-era luminosity label. Fixed — M5, frozen replay. Fixed. A category absent from the record is skipped with a MinorsTaken: One finding beyond the review, found while checking the Not changedRequiring
Five items deferred as design decisions rather than fixes, and I would rather Docs items are in HH_bbWW #107 and will go there. |
The formatting check installs the newest black, which is now 26.5.1 and applies the 2026 stable style; the newest release that still supports the Python 3.9 I had locally is 25.11.0, which left this alone. One blank line between the imports and a module-level comment. Checked with both versions afterwards. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
|
@cms-flaf-bot please test
|
|
pipeline#15653228 started |
kandrosov
left a comment
kandrosov
left a comment
There was a problem hiding this comment.
Reviewer note: this re-review was performed by Claude (Anthropic's AI, running in Claude Code), covering 71b010d..9363a84 (13 files, +535/−123). Per the request to focus on blocking and major issues only, style, docs and design preferences are left out. Each of the six round-1 findings was re-checked by executing the fixed code rather than by reading it; every candidate regression in the fix round was then put through three independent refutation passes, and one of the two that reached that stage was refuted and is not reported. Line numbers refer to 9363a84.
Verdict
All six findings are fixed, and the fix round introduces no blocker and no major defect. On correctness and robustness grounds this is ready to merge. One residual gap and four minors are listed below; only the first is worth a moment, and it is a three-line change that can equally follow in a separate PR.
The integration pipeline (#15653228) and all three GitHub checks pass on this head.
The six fixes, as verified
B1 — degenerate slices. Fixed, and confirmed without regression. Re-running the round-1 reproducer against the fixed code: the exhausted case now yields [None, None, None, (1, 10)] and discover_binning returns None; extend_outer_edges correctly leaves a None-bearing list untouched; rebin_hist_2d raises on a hand-made (0, -1). The underlying ROOT hazard was re-confirmed independently (IntegralAndError(0, -1, 1, 5) = 50.0 on a histogram whose true total is 50.0). Critically, the guard does not over-trigger: with a realistic peaked signal, n_slices 4/3/2/1 all still produce well-formed, contiguous slices covering [0, nx+1]. Replaying a binning.json written by the old code is also safe in both of its shapes — [(0,-1), (0,0), …] raises with nothing written, and [(0,0), (1,1), (2,2), (3,51)] is accepted but is not a duplication (the (0,0) slice is an empty underflow-only category, and the four slices sum to exactly the true full-plane yields).
M1 — multi-value lnN on merged processes. Fixed, and the algebra is right. Since the merged yield varies as Σᵢ yᵢ(1+vᵢ) = T + Σᵢ yᵢvᵢ, weighting over the full total with uncovered subprocesses contributing at zero variation is exactly the merged-process variation, and the previous divide-by-covered-yield did overstate it. Running DatacardMaker on the unmodified shipped x_hh_bbww_DL_run3.yaml now emits PDF_alphas lnN … 0.977/1.023 and top_mass lnN … 0.979/1.022 on the TotalBkg column in both the meta-era and standalone builds, where both were - before; the values reproduce by hand from the subprocess yields. The convention survives end to end: getUncertaintyForProcess → signed relative shifts → _lnNComponent → valueToMap → combine's kappa_down/kappa_up, matching _applyLnNToHist's 1±v, so the plain-era and meta-era paths now genuinely agree. (One cosmetic consequence: a merged process whose subprocesses all carry the same symmetric value gets 0.977/1.023 where an unmerged one gets 1.023 — equivalent to O(v²).)
M2 — limits.npz double-counting. Fixed, and the alternative taken is sound. Executed against all three shipped configs: CI_card.yaml keeps the same joint fit over its four standalone eras; the 1D config, which lists the group and its members, now yields top_level == [Run3_Early], combining == False, and a headline taken from the group's own merge — no path combines a group with its members. Two mechanics that could have undermined it were checked and are safe: the unsorted glob.glob cannot destabilise task identity across luigi's run() restarts, because dhi normalises datacards to a sorted tuple in modify_param_values (which runs on every instantiation, not just CLI); and the per-era merges cannot collide on one output, since store_parts["inputs"] is datacards_<hash(datacards)>. Worth recording that no task in the graph reads limits.npz or era_limits — both plot tasks consume the datacard mirrors — so the changed semantics carry no downstream risk.
M3 — HTCondor control-file collision. Fixed. datacard_era on StatInferenceTask is period for every task that does not declare meta_era, so no existing production's store path moves; the submission and status files live under local_path()/store_parts, which now separates the meta instance from its member. (One residue is noted below.)
M4 — meta-era luminosity label. Fixed. --year self.datacard_era with --period left as a real era is correct: HistPlotter uses year only to select config/plot/{year}.yaml, and everything era-real still flows through --period. config/plot/Run3_Early.yaml is now actually read.
M5 — frozen replay. Fixed in both directions that were reported: a category absent from the record is skipped with a reason, the content gates are bypassed under replaying, and a recorded category whose signal histogram is missing raises. See the one gap below.
Residual gap — worth three lines, not a merge blocker
bin_opt_2d/rebin_2d.py:928 — the cat_dir guard short-circuits the new replay contract.
cat_dir = sources[0][1].Get(...) / if not cat_dir: print(...); return at lines 928–931 runs before the if replaying: block at line 956. The consequence is an inversion: a category that is in the frozen record but whose whole <channel>/<category> directory is absent from the new input is skipped with a log line and exit 0, while the strictly milder case — directory present, signal histogram missing — raises at line 970. The input change that removes the most is the one that stays quiet, and the docstring one line below states the opposite contract ("A replay reproduces a recorded category set exactly, or it is not a replay").
Reproduced independently twice: a record holding OS_Iso/res2b and OS_Iso/recovery, replayed over an input carrying only res2b, prints one [skip] line, exits 0, produces only the res2b slices, and writes a new record narrowed to ['OS_Iso/res2b']. Nothing downstream catches it — maker.py:1024 drops a category with no signal shape without raising, and maker.py:1370 skips its per-category card — so the published limit would be set on fewer categories than the frozen record specifies.
Severity is bounded honestly, and this is why it is not called a blocker: --binning is reachable only from the command line. Every shipped preprocess: block passes --binning-config and never --binning, so replaying is False on every chain path including CI. The fix is to move the cat_dir check after the if replaying: block (or raise there when replaying and frozen is not None), so the absent-directory case joins the missing-histogram case.
Minors (one line each, listed for completeness)
law/ResonantLimitsTask.py:161—merged.get(top_level[0])indexes before theNonecheck, soget_top_level_eras() == []gives a bareIndexErrorafter every era's cards and fits have already run, and theRuntimeErrortwo lines below that exists to explain exactly this can never fire. Reached by a natural edit of the shipped 2D config: its four sub-eras are commented out withera_groupsleft declared (and a comment saying it stays "regardless"), so listing the members without the group produces an empty top level. No shipped config triggers it as it stands.bin_opt_2d/rebin_2d.py:874— the new invalid-range guard coversx_rangebut noty_ranges, which reach the sameIntegralAndErrorone line later; verified thaty_ranges: [(0, -1)]still returns the full y axis (200.0) with no raise. Not reachable from generated records, since no producer inverts a y range, but it is the same ROOT reinterpretation the guard exists to close, andrecord_to_slicestakes y ranges verbatim from an external file.bin_opt_2d/rebin_2d.py:1114— a replay writesslice_varfrom the current knobs rather than from the record it replayed, so the newbinning.jsoncontradicts the shapes beside it (slice_var: xnext to directories titled0.80 < DNN < 1.00). The sibling key one line below takes the effective value; the edges themselves round-trip correctly.law/StatInferenceTask.py:88—store_partsnow separates the two instances, but FLAF'sremote_log_dir_target()is keyed onself.period(FLAF/run_tools/law_customizations.py:349), so a meta-era and its member instance still stage HTCondor logs to the same remote file and one overwrites the other. Products and completeness are unaffected — this costs a diagnostic, not a result — and the fix belongs in FLAF rather than here.
One correction to the fix round's own reasoning
The DhiPlotMixin change is right and strictly safer than the presence-only form, but the stated rationale does not hold: under EXPLICIT_PARSING luigi sets nargs="?" and const=True (both the pinned 3.2.1 and 3.7.3), and law is imported before the dhi task classes are defined, so a bare --y-log already evaluated to True. The shipped log-axis plots were therefore not being drawn linear. What the change genuinely fixes is the False case from round 1, where a value was dropped from the command while plot_targets built the task with it — so the change should stay, and no dhi parameter overrides parsing, which makes the --flag True form safe everywhere.
On the items not changed
The four rejections are accepted. Requiring processes: on a multi-value lnN would indeed delete lumi_1_13p6TeV, whose era-only scope is deliberate; the bbtautau QCD_norm exposure is a config problem in a file this PR does not touch and is dead today, and an issue of its own is the right home. The README.md and argparse points are fair.
The five deferrals are also reasonable at this bar: each is either a loud failure, a documented path, or a refactor, and none can produce a silently wrong result. The getCombinedShape / hasNominalShape contract is worth a follow-up issue so it is not lost, since partial sub-era availability currently crashes the card build rather than being decided either way.
Coverage note
CI still exercises only the standalone-era path at n_slices=1 with --workflow local, so the meta-era plumbing, both plot tasks, TotalBkg merging and multi-slice binning remain unexercised by the pipeline. The fix round's own evidence — a full Run3_2022 rebinning showing 4 degenerate slices before and 0 after with all 77 surviving categories keeping byte-identical edges, and Run3_Early cards rebuilt before and after M1 with the limit unmoved — covers the substance of that gap for these changes.
cms-flaf/StatInference#20 (HistRebinTask, limit plotting, meta-era support) is squash-merged as f43a4b6, so the branch commit 71b010d this pinned is no longer reachable from any upstream branch. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
* Stash pop the limits changes
* Update global
* Update global again ):
* Remove puppiMET cut region
* Update datacard config
* Formatting
* Add the run 2 DL external limits
* Add early run3 plotting config
* Update cards
* Update inference and CI card
* Add 1D binning option, add hepdata results
* Add the brazillian flag plot back in
* Gate the DNN binning per process and merge the boosted backgrounds
DY has no usable MC statistics in the b-tagged same-flavour categories: over the
232 eE/muMu SR slices of Run3_Early its effective MC count has a median of 5.8 and
sits below the binning's own min_bkg_neff of 4 in 42% of them, with five slices
going negative outright and blocking the datacards. TT, ST and VV are all fine
(median N_eff 471 / 211 / 97).
Two changes, both in x_hh_bbww_DL_run3.yaml:
- min_dnn_bkg_each: 0.01 requires every non-negligible background to be positive in
every DNN slice. min_bkg_neff only ever gated the *summed* background, which never
binds -- a background that had fluctuated negative was hidden inside a healthy
total. min_dnn_bkg_neff is available but left at 0: requiring those backgrounds to
be measured rather than merely positive costs 35% of the Asimov Z in muMu/res2b at
m500.
- TotalBkg merges TT/DY/ST/VV into one template in SR/boosted, all three channels.
The gate fixes slice integrals but eE/SR/boosted_dnn1 still has negative *bins*
inside a positive slice, and quantile mass binning makes every bin of an n<=5
slice signal-carrying, so the unconditional rejection fires. eMu gains DY, which
its own channel list had excluded from that channel's background entirely.
Validated by rerunning the full chain on v2605a (rebin -> datacards -> limits):
negative shapes 5 -> 0 of 1392, datacards build under the unmodified
resolveNegativeBins, and the expected limit moves by +1.46% geometric mean over ten
mass points (+4.8% worst, at 700 GeV). Split: binning gate +0.22%, merge and strict
acceptance +1.24%.
Bumps StatInference to 44b27b1.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Bump StatInference to ede2117
Carries the slice-naming cleanup: the DNN selection each slice stands for is now
the slice directory's own title rather than a dnn_slices.json side-car, so there
is no path for a reader to be handed wrongly and no key name for producer and
consumer to agree on. Directories are also created a level at a time, fixing the
stale parent titles nested mkdir left behind ("muMu" was titled
"muMu/SR/res2b_dnn0").
Datacard-neutral: rebuilt from a fresh rebin of all four sub-eras, the datacard
.txt files are identical and all 40992 shape histograms unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Drop the raw HEPData record from the PR
config/Datacards/run2_hepdata_radion_results.json is the unprocessed HEPData
submission for HIG-21-005 (7.4 kB: doi, headers, keywords, location). Nothing
reads it. The file the limit plots actually consume is the digitised
run2_spin0_DL_external_limits.json, referenced six times across
x_hh_bbww_DL_run3.yaml and x_hh_bbww_DL_run3_1D.yaml via external_limits, and
that stays.
The converter that turned one into the other, StatInference's
hepdata_to_external_limits.py, was already dropped from that PR for the same
reason. Recoverable from de37048 if the digitisation ever needs redoing.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Name the DNN slices from the config, not from StatInference
StatInference no longer hardcodes how a 2D shape's slices are named: the pattern
comes from the datacard configuration's `binning:` block, and the shared code
defaults to a neutral "{base_category}_slice{slice_idx}" that names no
discriminant. Pin our own here so the datacard bins keep the names the rebinned
shapes already on EOS carry, and so the slice directories are labelled with a DNN
range rather than an anonymous "x" one.
Bumps StatInference over the review fixes that introduced this (CategoryNaming
replacing the module-level format string, and the split of law/tasks.py into one
module per task).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Follow StatInference's rename of the binning knobs
StatInference no longer names either axis of the 2D->1D transformation: the
sliced axis is "slice", the rebinned one is "bin", and which variable sits on
each is the analysis's business. Rename the keys accordingly.
n_dnn_slices -> n_slices
min_dnn_bkg_sum -> min_slice_bkg_sum
min_bkg_neff -> min_slice_bkg_neff
min_dnn_bkg_each -> min_slice_bkg_each
min_dnn_bkg_neff -> min_slice_bkg_each_neff
max_mass_bins -> max_bins_per_slice
bkg_per_mass_bin -> bkg_per_bin
min_mass_bkg_neff -> min_bin_bkg_neff
min_mass_bkg_each -> min_bin_bkg_each
Values are untouched; only the keys move, here and where the surrounding comments
name them. The comments go on saying "DNN slice" and "mass bin" because in this
configuration that is what they are -- this file is where naming our own axes
belongs, which is the point of the change upstream.
Required, not cosmetic: config_binning() raises on a key it does not recognise,
so these two files have to move in step with the submodule.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Follow the binning moving out of the law chain
StatInference no longer rebins as part of the datacard chain: the 2D->1D step is a
standalone pre-step, StatInference/bin_opt_2d/rebin_2d.py, and CreateDatacardsTask reads
whatever 1D shapes it is pointed at.
x_hh_bbww_DL_run3.yaml therefore drops its `binning:` block -- those knobs now live in
StatInference/bin_opt_2d/binning.yaml -- and lists the twelve sliced categories it
actually has instead of the three base ones the chain used to expand. category_pattern
moves alongside them as a top-level key, because grouping the slices of one base category
for the per-category limits is the one thing the datacard step still needs it for.
Nothing about the binning itself changes: the values move to binning.yaml unaltered, and
the categories listed are the names the current rebinned shapes on EOS already carry. To
build cards from them, point --hists-version at a rebin_2d.py production.
CI_card.yaml switches to the 1D per-mass DNN score (DNN_M300_Signal). It was pointing at
the 2D DNN-vs-HME plane and relying on HistRebinTask to flatten it, which no longer
happens in the chain; at --test statistics the rebinning's production gates would drop
every category anyway. So CI keeps covering the datacard and limit steps end to end, and
stops covering the binning -- worth knowing rather than assuming, and worth a separate
rebin_2d.py invocation in the integration pipeline if we want that coverage back.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Bump StatInference: defer_negative_bin_check removed
The meta-era negative-bin deferral is now structural (_rawShape / getShape) rather than a
boolean parameter. Shapes and datacards are unchanged -- the two implementations were run
side by side over twelve synthetic cases and agree exactly.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Bump StatInference: rebin_2d was rebinning nothing
The datacard config lists the sliced category names; rebin_2d looked those up in the 2D
input, which is keyed by the base categories. Every category was skipped and the script
exited 0. Verified against the real Run3_2023BPix shapes: 467712 histograms, 3548160 bins,
bit-identical to the pre-move script.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Bump StatInference: rebinned shapes plotted by FLAF's HistPlotter
dc_make/plot_rebinned.py is gone; CreateDatacardsTask drives Analysis/HistPlotter.py with
the sliced categories as channel:category:region keys. signal_plot_scale in global.yaml is
still honoured -- HistPlotter reads that same key itself.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Bump StatInference: ResonantLimitsAndHistPlotTask now includes the limit plots
It required ResonantLimitsTask and stopped at the limits; it now requires
PlotResonantLimitsTask, so one law run produces the limits, the limit plots declared in
the config's limit_plots, and the input histogram plots.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* Declare the 2D rebinning as a preprocessing step
The rebinning has been outside the law chain since the binning moved out of it, which
left the analysis with a manual pre-step: run rebin_2d.py by hand, then point
--hists-version at whatever it wrote. StatInference now offers PreprocessShapesTask, a
generic hook that runs whatever the datacard configuration's `preprocess:` block names,
so the chain does that step itself and CreateDatacardsTask reads its output directly.
This declares it. The block names the script and the arguments it wants; the task
supplies --input, --output, --era and --config and knows nothing else about it. An
analysis that declares no `preprocess:` block skips the task entirely and reads the
merged histograms as before.
The binning knobs move here as config/Datacards/binning_2d.yaml -- slice count, bin
budget and the yield and effective-entry floors were always analysis configuration, and
sat in StatInference only because that is where the script lived. The binning the script
derives is a product, not configuration, so it is written into the task's output on EOS
beside the shapes it produced, not committed here.
The four sub-eras are commented out of `eras:` for now. None builds standalone: on one
era's statistics a few DY shapes go negative beyond tolerance in the sparsest DNN slices
and resolveNegativeBins rejects them (2022: 5, 2022EE: 5, 2023: 2, 2023BPix: 6,
Run3_Early: 0, over 4 mass points of ~500 background shapes each). They stay in
era_groups: Run3_Early is a statement about the data, not about which limits we set.
Bumps StatInference to 9c39616, which also restores ResonantLimitsAndHistPlotTask's
dependency on ResonantLimitsTask rather than PlotResonantLimitsTask.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
* Bump StatInference: isinstance checks, and the shape-plot gap fix
ebcb813 answers the review comment on dc_make/maker.py -- the unc.type checks now ask
what the uncertainty is with isinstance rather than comparing the enum its own subclasses
derive -- and fixes shape plotting for a mass point where the preprocessing gated a
category out. At MX=300 the boosted categories fall below the signal threshold, and
passing them to HistPlotter anyway cost the recovery panels and all three grids.
Also carries 9c39616, restoring ResonantLimitsAndHistPlotTask's dependency on
ResonantLimitsTask rather than PlotResonantLimitsTask.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
* Point at rebin_2d.py directly, not the wrapper script
Bumps StatInference to 6521ffb, which drops bin_opt_2d/call_rebin_2d.sh. The chain and
the datacard configuration are the supported way to run the rebinning; the wrapper only
existed while there was no task to do it, and carried a second copy of the argument
contract plus a hardcoded production version.
The docs give the direct invocation instead, so the by-hand route stays documented
without a script to keep in step.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
* Declare the impact plots, and bump StatInference for the task
StatInference gains PlotPullsAndImpactsTask, which wraps dhi's PlotPullsAndImpacts the
way PlotResonantLimitsTask wraps the limit plots, so the impacts land on EOS beside
everything else instead of only under inference/data/store. This declares them.
Masses are listed one at a time deliberately: PullsAndImpacts is a per-parameter workflow,
about two combine fits per nuisance per mass, so ~150 fits for a single point at this
card's 76 nuisances.
Also bumps StatInference to 43a4131, which additionally stops a meta-era turning an
era-independent lnN into a flat shape template -- QCD_scale_*, PDF_alphas and top_mass
stay true lnN in the Run3_Early card now, as they already were in a standalone-era one.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
* Give the CI its single-bin reduction back, through the preprocessing step
The CI has always fed the datacard maker the 2D DNN-vs-HME plane and reduced it to
essentially one bin somewhere: first at datacard time, through hist_bins pointing at
CI_binning.json, and then in the chain, once HistRebinTask existed. de37048 dropped the
first when the second made it redundant.
Moving the binning out of the law chain removed the second, and dc91fcf pointed CI_card
at the 1D DNN score without putting a reduction back in its place. So the CI has been
handing combine the native per-bin distribution of a --test run -- 1000 events per
dataset, every yield ~1e-3 of nominal, most bins empty or negative -- which combine will
not fit.
The reduction returns where it now belongs: CI_card declares a `preprocess:` step, and
CI_binning_2d.yaml gives rebin_2d.py one slice, one bin and every gate at zero. The CI
therefore exercises the chain that actually runs -- PreprocessShapesTask included --
rather than a datacard-time Binner path the production configuration no longer uses.
Only the knobs differ, and only because at --test statistics the production floors would
reject every category before a shape was written.
CI_card's categories become the sliced names the single slice produces
(OS_Iso/res2b_dnn0, OS_Iso/recovery_dnn0) with the matching category_pattern.
Verified: the graph resolves CreateDatacardsTask -> PreprocessShapesTask ->
MergedHists(variable=DNN_m300_vs_HME), the same shape as production; the declared
categories equal bases x n_slices for both the CI and the production card; and the
category_pattern agrees between each card and its binning file.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
* Rank the impacts at 300, 600 and 800 on the combined card
The combined card is the ranking that describes the result -- every era and every
category in one fit -- so that is what the default asks for, at three mass points across
the scan rather than one arbitrary middle.
Bumps StatInference to 2414124, which lets an entry carry a `glob:` addressing one
category's card instead. The default does not use it, but per-category rankings are a
question worth being able to ask: a nuisance that dominates res2b need not dominate
boosted. Documented with a worked example.
Three masses is about 480 combine fits, since PullsAndImpacts runs one job per parameter
per direction. That is why masses are listed rather than taken from the model.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
* Run the impact fits on HTCondor
One combine job per nuisance per direction, ~160 per mass point at this card's 79
nuisances, and they are independent -- so they belong on the batch rather than in series
on a login node. Bumps StatInference to dd10fdd, which added the dhi_args pass-through
this needs: the workflow knob is on PullsAndImpacts, not on the plot task.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
* Do not claim the impact fits run on HTCondor
40a0607 set --PullsAndImpacts-workflow htcondor as the default. It does not work: dhi's
condor submission depends on BundleCMSSW and BundleSoftware, whose complete() reads
CMSSW_BASE and DHI_SOFTWARE_FLAG_FILES straight from the environment. env.sh sets neither
-- it exports DHI_STORE and DHI_SOFTWARE but not DHI_BASE or the flag files -- and FLAF's
cmssw_env has CMSSW_BASE but none of the DHI variables. The submission therefore dies in
complete() before a job is written, and the task simply stalls after CreateWorkspace.
The full environment exists only inside the cmsEnv subshell (FLAF/cmsEnv.sh, the alias
env.sh defines), so making this work means routing the dhi command through it and
checking the plotting still behaves there. Until that is done and tested, the fits run
locally, and the block says what would be needed rather than pretending.
The dhi_args mechanism itself stays -- it renders correctly and law accepts the
namespaced arguments; it is the environment underneath that is not ready.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
* Correct what an impact mass point actually costs
I had it that PullsAndImpacts always fits each nuisance separately, so a mass costs ~160
combine jobs and the three here ~480. That is only true of `method: default`.
create_branch_map is explicit about it:
branches = [self.pois[0]]
if self.method == "default":
branches.extend(params)
With `robust`, which is what this configuration uses, the branch map is the nominal fit
alone and robustHesse reads every impact off the inverted Hessian. All three masses ran
in minutes, one fit each, and produced 72-74 ranked parameters.
Which also means a batch system buys nothing here -- there is one fit per mass to
parallelise. That only becomes worth doing under `method: default`, so the note about
HTCondor now says so rather than implying the current setup is slow.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
* Rank per uncertainty, at the limit
method: default, so each nuisance gets its own pair of fits. `robust` takes a single fit
and inverts the Hessian instead, which is far cheaper but drops whatever it cannot invert
-- on these cards it silently lost all four CMS_scale_j_*, 5 to 7 nuisances per mass, and
still reported success. A ranking that is missing the jet energy scale is not a ranking.
poi_value: limit, so the Asimov is built where the measurement is rather than at dhi's
r=1. Bumps StatInference to 927348e for both.
This makes the fits genuinely expensive again -- about two combine jobs per nuisance per
mass, ~160 here, ~480 for the three masses -- which is exactly the case a batch system is
for, and HTCondor submission from this task does not work yet.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
* Fall back to Simplex on the impact fits
Without it the run dies: Migrad fails on CMS_eff_m_id_iso_2023 at 300 from a bad seed
gradient, and dhi aborts the whole merge, so 80 converged fits produce nothing. With it
that fit converges and all three masses complete, each ranking all 79 card nuisances.
Bumps StatInference to 71b010d for the fit_args pass-through. Also corrects the cost note
now that it has been measured: `default` is one fit per parameter, ~80 per mass at
20-30s, so about an hour for the three -- not the two-per-nuisance I had assumed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
* Point StatInference at main now that the rebin PR is merged
cms-flaf/StatInference#20 (HistRebinTask, limit plotting, meta-era
support) is squash-merged as f43a4b6, so the branch commit 71b010d this
pinned is no longer reachable from any upstream branch.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
---------
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
The code this branch carried is already on main: the review of cms-flaf#20 landed the same two mechanisms under different names -- `uncAppliesTo` for what was `uncAppliesInEra`, and `getMVLnNValue` for `multiValueUncInEra`, which merges the sub-eras it finds a value for weighted by their yields rather than taking the first. `getCombinedShape` likewise already varies only the matching sub-eras and takes the rest at nominal. Rebasing onto main therefore left nothing to change in `dc_make/maker.py`. What main does not have is any statement of the convention, so all that remains here is the README section: that a meta-era bin holds the summed sub-era histograms and so resolves an `eras:`-scoped nuisance while summing; that the nuisance name is the histogram name, which puts the era suffix in the producer's `weights.yaml` rather than in a `decorrelate:` key on the datacard; and that a sub-era the uncertainty does apply to is still required to have its variation, so suffixing one half without the other fails loudly instead of silently dropping the nuisance. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
No description provided.