Add hist rebin limit plots - #20
Open
aebid wants to merge 27 commits into
Open
Conversation
Author
|
@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 |
Author
|
@cms-flaf-bot please test
|
|
pipeline#15504166 started |
Author
|
@cms-flaf-bot please test
|
|
pipeline#15504367 started |
|
pipeline#15504367 passed |
|
pipeline#15504166 failed |
Author
|
@cms-flaf-bot please test
|
|
pipeline#15509624 started |
|
pipeline#15509624 failed |
Author
|
@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:
Author
|
@cms-flaf-bot please test
|
|
pipeline#15512406 started |
|
pipeline#15512406 passed |
kandrosov
requested changes
Aug 13, 2026
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.
aebid
force-pushed
the
Add_HistRebin_Limit_Plots
branch
4 times, most recently
from
August 13, 2026 21:34
1e5b993 to
71fb60d
Compare
kandrosov, on dc_make/plot_rebinned.py: "This code shouldn't be here. We need to add support for rebinning for the plot task in FLAF." FLAF already plots these. Analysis/HistPlotter.py -- the script HistPlotTask runs -- takes `channel:category:region` keys and plots whatever it is handed, so the sliced categories need nothing on the FLAF side: CreateDatacardsTask just passes "eE:res2b_dnn0:SR" and points --inFile at the shapes. 567 lines of plotter delete, and these plots now come out in the same style as every other plot in the analysis, from one implementation. Everything plot_rebinned.py had reimplemented comes back for free by handing over --globalConfig: the process list and colours, signal_plot_scale (HistPlotter reads the same global.yaml key), the blinding check, the background uncertainty band and ratio panel. The axis title is no longer hardcoded or looked up by hand -- histograms.yaml describes the 2D variable as `var_list: [x, y]`, so plot_variable() takes the y entry and the plotter reads its own metadata from there. A configuration whose input was always 1D gets the variable itself. The sub-eras are hadd'ed into one file first, because the datacard bin is their sum (getCombinedShape does the same at card-build time) and plotting them separately would show something the fit never sees. No --rebin: these shapes are already at their final binning, and it would coarsen them back onto the histograms.yaml grid. What is lost is the grid layout -- one page per base category with the slices across and the channels down. It is now one plot per channel and category, 36 for this configuration. That layout was the only thing plot_rebinned.py did that FLAF does not, and it cost a PanelBackend subclass reaching into PlotKit's MplhepBackend internals to draw into an axes it did not own, which is most of why the file was 567 lines. Verified against the real Run3_2023BPix rebinned shapes: all 36 plots render, with the CMS styling, the "Deep HME Mass" axis label out of histograms.yaml, the channel/category/ region labels, the signal overlays and the uncertainty band. Spot-checked eE/SR/res2b_dnn3 at MX=700 against binning.json -- the single flat bin in the plot is the binning that was actually chosen there (0.7 background events, bkg_per_bin 5.0), while dnn0 has its 10 bins clustered at 420-750 GeV around the resonance. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
kandrosov: "Since we now have PlotResonantLimitsTask, we should use it." The task existed to give one `law run` that produces everything an analyst wants, but it asked for the limits and stopped there, so the overlay and band plots the configuration declares in `limit_plots` had to be requested separately. It now requires PlotResonantLimitsTask, which pulls ResonantLimitsTask in behind it -- naming the limits here as well would only be a second path to the same task. Also renames get_eras() to get_plottable_eras(). It returned the *opposite* set to ResonantLimitsTask.get_eras() under the same name: this one drops the meta-era names and keeps their members, since HistPlotTask resolves `period` against config/<era>/ and a meta-era has no such directory, while ResonantLimitsTask keeps the eras that carry their own datacards. Two sibling tasks with a same-named method returning complementary sets is what makes a configuration listing both the sub-eras and their group look like it double counts, when the two consumers in fact partition that list exactly. Verified: the graph resolves ResonantLimitsAndHistPlotTask -> PlotResonantLimitsTask -> ResonantLimitsTask -> CreateDatacardsTask, plus one HistPlotTask per real era, and law index still reports 76 task families. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every era listed in the datacard configuration's `eras:` should get its own binning
and its own limit, and the previous interface made that impossible to express: the
caller chose --era and --discovery-eras independently, so a standalone binning and a
combined one both landed at <output>/<era>/ and the second silently overwrote the
first. Running the four sub-eras with the combined discovery set -- which is what a
production actually wants -- produced four *identical* binnings, none of which was a
standalone binning of anything.
--era now names an era of that list and decides everything:
a plain era binned on its own statistics, for a standalone limit
a key of era_groups edges discovered from its members' summed statistics, then
applied to each member separately
<output>/Run3_2022/Run3_2022/<var>/<var>.root standalone
<output>/Run3_Early/Run3_2022/<var>/<var>.root combined edges
<output>/Run3_Early/Run3_2022EE/... (one file per member)
The target era is the outer level precisely because the same source era appears under
several of them with different edges. --discovery-eras is gone: deriving it from
era_groups is what makes the two impossible to mismatch.
A group era's members are *not* summed here. Summing would make a per-era lnN
inexpressible -- getMetaEraLnNShapeUnc builds lumi_13p6TeV_2022 by scaling each
sub-era's own shape and re-summing, which needs that sub-era to still exist as a
shape. The datacard step goes on doing the combination, as it already did.
Verified against the real v2605a_DNNOutputs_v3 shapes, standalone Run3_2022 vs the
combined binning, eE at MX=700 -- the combination affords more bins throughout, which
is the whole reason the two cannot share a path:
category standalone (2022) combined (Run3_Early)
SR/res2b 9,1,1,1 x=-,3.6,5.4,6.0 10,6,1,1 x=-,3.9,6.3,6.9
SR/boosted 1,1,1,1 x=-,-3.9,-3.0,-2.1 2,1,1,1 x=-,-2.1,-0.6,0.6
SR/recovery 10,6,1,1 x=-,2.1,4.8,5.1 10,6,6,1 x=-,3.6,4.5,6.3
The signal a category is judged on differs by the same factor: muMu/SR/boosted at
MX=300 sees 0.016 events standalone against 0.130 combined.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Replacing plot_rebinned.py with HistPlotter.py gained the analysis's own styling and one plotting implementation, but lost the layout it had: a page per base category with the slices across and the channels down. The slices of a base category are one physical selection cut into pieces and the fit sees them together, so a slice that is reasonable in eMu and pathological in eE is obvious side by side and invisible across 36 files. The layout comes back without the plotting doing. HistPlotter.py still draws every panel; _stitch_grid does page geometry only -- pypdf, one blank sheet, a translate per panel -- so there is still nothing in StatInference that knows how to draw a histogram. The per-category PDFs are kept alongside the grids, since they are what a closer look at one bin wants. A skipped slice leaves its cell blank rather than shifting the row, so column N stays under column N across channels; that matters because the binner skips a category for some masses (boosted at low MX) and a shifted row would silently compare different slices. An unsliced configuration gets a single-column grid of channels, which is the same code path with one column. Verified on the 340 panels from the end-to-end Run3_Early run: three grids, one per base category, each 4 slices x 3 channels, with the yield falling left to right as the DNN tightens and the mass binning collapsing to one bin in the tightest slices -- matching the binning.json for those categories. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rebin_2d.py was writing into a Hists_merged tree so that the existing MergedHists task
would resolve it without a law change. That was expedient and wrong: these are not
merged histograms, and calling them that hides which of the two a production holds.
RebinnedHists is the sibling task, reading
<hists-version>/Hists_rebinned/<target_era>/<period>/<var>/<var>.root
The path carries two eras and they are different things. `target_era` is the era whose
binning this is; `period` is the era whose events these are. They match for a standalone
era, and for a group era every member appears under the group's binning -- which is
exactly why the target era is a directory level and not part of the version, since the
same period appears under several binnings.
Which tree to read is stated in global.yaml's StatInference block (`rebinned: true`)
rather than inferred. The two differ in shape as well as name, so a wrong guess reads a
different binning than the one asked for and nothing downstream would notice.
stage_inputs() is untouched: rebinned_hist_reqs() is keyed (era, variable) like
merged_hist_reqs(), so either lays out as "<era>/<var>/<var>.root" and the datacard step
does not know which it got.
Verified: the meta-era card resolves
/HV/Hists_rebinned/Run3_Early/Run3_2022EE/DNN_m1000_vs_HME/... and a standalone card
/HV/Hists_rebinned/Run3_2022/Run3_2022/..., both matching what rebin_2d.py writes; the
graph resolves through RebinnedHists to full depth; law index reports 77 task families,
the one addition being RebinnedHists.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
It was going to the --output root, shared by every target era of a production, which
took a merge on write to stop each era clobbering the last. That was solving a problem
that should not exist: one era's binning has nothing to say about another's, since they
are derived from different statistics and describe different shapes.
Each era's record now lives in its own directory beside the shapes it describes:
Hists_rebinned/Run3_2022/binning.json
Hists_rebinned/Run3_Early/binning.json
Nothing can collide, so the merge goes away; re-running one era cannot disturb another;
and --binning replays a record by pointing at the era it belongs to rather than at a
file holding several.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rebin_2d.py was deriving a binning and then writing a whole second histogram production
from it. Everything awkward in the last few commits came from that second product
existing: a Hists_rebinned tree, a RebinnedHists task, a `rebinned: true` key so the
datacard step knew which tree to read, a target-era directory level so two binnings of
the same era would not collide, and a binning.json that could disagree with the shapes
beside it.
None of that is needed. The binning is the product:
rebin_2d.py --era Run3_Early -> <output>/Run3_Early/binning.json
and dc_make/binner.py applies it to the merged 2D shapes when the cards are built. The
configuration consumes it through `hist_bins`, which has always meant "a JSON describing
the binning" -- so the standalone/combined choice is which file hist_bins points at, a
path rather than a boolean, and CreateDatacardsTask is back to one input tree that it
does not have to be told about.
This is what the review asked for: "At most, it should apply existing rebinning (provided
via a JSON file) to the inputs." Binner could not express a 2D->1D cut before, which I
had taken as a reason to write shapes instead; it was a reason to teach Binner the cut.
It is ~40 lines, and it plugs into a seam that already existed -- applyBinning() is
called at every shape-loading site in maker.py with exactly (era, channel, category,
model_params, hist), so _rawShape and the getShape split are untouched.
Deleted: law/RebinnedHists.py, rebinned_hist_reqs, reads_rebinned, the Hists_rebinned
tree, the `rebinned: true` key, and rebin_2d's shape-writing (mkdir_titled,
rebin_hist_2d, the per-source output files).
Also faster, by a lot: deriving Run3_2022's binning takes 15 s where writing its shapes
took ~5 min. The cost was never the optimisation, it was 170 MB of intermediate ROOT.
Verified against the real v2605a_DNNOutputs_v3 shapes: the Run3_2022 binning applied to
eE/SR/res2b at MX=700 gives 9,1,1,1 bins with the recorded edges, and across all nine
channel x base-category combinations the four slices partition the 2D histogram exactly
-- slice sum equal to the 2D integral including under/overflow to every digit, so nothing
is lost or double counted. law index is back to 76 task families and the graph resolves
through MergedHists alone.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
| ): | ||
| continue | ||
|
|
||
| if self.isMetaEra(era) and unc.type == UncertaintyType.lnN: |
Three things, all found by running the chain rather than reading it.
Memory. TFile.Get() hands back an object ROOT keeps alive for the lifetime of the file,
so dropping the Python reference frees nothing. That was affordable while the datacard
step read the small 1D shapes a bin is made of; it stopped being affordable when the
binning moved to datacard time and the step began streaming the 2D inputs it is cut
from, at ~240 KB each. One era's build reached 14.6 GB resident and pushed the node into
swap. Every read now goes through DatacardMaker.readHist(), which takes ownership so
refcounting frees each input once it has been sliced, while anything still referenced --
a cached shape -- lives exactly as long as before. Measured over five mass points and
nine channel/category combinations: 0.93 GB against 14.6 GB, and what remains grows with
the retained shapes rather than with the reads.
Histogram paths. hasNominalShape and the shape loaders built paths from the datacard bin
name, which is a slice ("SR/res2b_dnn0"). The input holds the base category it is cut
from ("SR/res2b"). Every lookup returned null, every process was skipped, and the cards
came out with imax 0 -- written, valid-looking, and empty. histCategory() makes the
distinction explicit: paths that *read* use the base, everything naming a bin keeps the
slice.
Uncuttable categories. A binning records nothing for a category it could not cut -- too
little signal to slice, typically boosted at low mass in a single era. addProcess booked
a bin there anyway and then failed looking for its shape. It now skips, which is the
judgement hasNominalShape already made for sparse single-era categories.
Era coverage now follows the datacard configuration with no flag to set. get_all_eras()
is every era listed, each of which gets its own cards, limit and plots; get_top_level_eras()
is the smaller set that may be *combined*, excluding a group's members because the group
already is their combination. Only the cross-era combineCards step uses the latter. The
--limit-era parameter added earlier is gone.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The 2D->1D rebinning has been in two places this session and neither worked. Inside the
chain as HistRebinTask it drew "we don't have a universal binning optimization routine...
keep these two steps separate". Outside it entirely, producing only a binning JSON that
Binner applied at datacard time, it put the analysis's binning model inside dc_make, made
CreateDatacardsTask stream 2D histograms at 14.6 GB resident, and left the shape plots
with nothing to read.
A configurable hook is the position that holds. The datacard configuration declares a
step; the task supplies --input, --output and --era and knows nothing else:
preprocess:
script: StatInference/bin_opt_2d/rebin_2d.py
args: [--binning-config, config/Datacards/binning_2d.yaml]
Nothing in this repository knows what rebinning is. An analysis needing some other
transformation writes its own script against the same three arguments, and one needing
none declares no block at all -- then CreateDatacardsTask requires MergedHists exactly as
before and the task is never scheduled. Verified both ways: with the block the graph is
CreateDatacardsTask -> PreprocessShapesTask -> MergedHists, without it the graph is
unchanged from today.
Being a task with a real output target is what removes the awkwardness. CreateDatacardsTask
calls self.input(); the --hists-version aiming, the Hists_rebinned tree name, the
`rebinned: true` key and the target-era directory level all existed only to work around
not having one, and all go.
The contract is the layout: whatever the step writes must be
"<era>/<variable>/<variable>.root", which is what input_file_pattern resolves against and
what the merged histograms already look like. So the datacard step cannot tell whether it
is reading preprocessed shapes or raw ones.
rebin_2d.py writes shapes again, restored from 3c7d0af, with binning.json beside them in
the same output. The target-era directory level from 72ed428 is dropped: the task's output
directory is already one era's, so repeating it there doubled the era. Everything else of
72ed428 stays -- --era names the era to produce and its sources come from era_groups, so a
standalone era is binned on its own statistics and a group era on its members' summed.
Where things live follows what they are. The knobs move to the analysis as
config/Datacards/binning_2d.yaml -- those numbers were always analysis configuration
sitting in StatInference by accident. The derived binning.json is written with the shapes
it produced, since it is a product of the run and not an input to it;
config/Datacards/binnings/ is deleted.
Removed with the datacard-time slicing it existed for: Binner.sliceOf/applySlice and its
format branch, maker.histCategory (the sliced-to-base mapping, which is right only when
the input is the raw 2D tree -- preprocessed shapes carry the sliced directories again),
hasNominalShape's binner consult, and addProcess's skip-when-no-binning. A category the
rebinning skipped now simply has no directory, which the original file.Get check already
handles. maker.readHist stays: taking ownership of what TFile.Get returns is correct
whoever reads 2D histograms, and that is now rebin_2d.py.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The Run3_Early cards carried 12 of the 76 nuisances the configuration declares. Every per-era `type: shape` was missing -- 64 of them, including CMS_scale_j_* and CMS_res_j_*, the jet energy scale and resolution, plus pileup, lepton ID, trigger, btag stats and the ak8 set. Not a reporting gap: they were absent from the fit, so that limit was optimistic by whatever they are worth. addUncertainty gated on unc.appliesTo(process, era, ...) with the literal "Run3_Early", and appliesTo matches that against the uncertainty's own `eras:` list, where it does not appear. False, silently, for all 64. Per-era lnN escaped only because meta-eras are intercepted for them one branch earlier, in _addMetaEraLnNAsShapeUnc -- which is why lumi_13p6TeV_2022 was in the card and CMS_scale_j_2022 was not. That asymmetry was the whole bug. uncAppliesTo() gives a meta-era its sub-eras' uncertainties, at both gates in addUncertainty (the plain and the parametric-process branch). uncertainty.py is untouched: answering False for an era it was never told about is right at that level, and the meta-era is a fact DatacardMaker knows because era_groups lives there. The gate alone would crash. getCombinedShape passed unc_name/unc_scale to every sub-era, but TT_CMS_scale_j_2022_Up exists only in Run3_2022's file. So a sub-era the nuisance is not scoped to now contributes its *nominal*, which is also what makes the result correct rather than merely present: the summed Up shape is 2022 varied plus three nominal, so it carries the full yield and its ratio to the nominal reflects only 2022's share. Summing just the eras holding the histogram would give a shape missing three quarters of the events and a meaningless ratio. Verified on the real configuration: CMS_scale_j_2022 and CMS_res_j_2023BPix go False -> True for Run3_Early while the correlated CMS_btag_LF stays True; and for CMS_scale_j_2022 exactly Run3_2022 contributes its variation with the other three contributing nominal, against all four for CMS_btag_LF. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
rebin_2d.py exits 2 without --config, so the chain died the first time PreprocessShapesTask actually ran it. The gap is not specific to rebinning: any step transforming these shapes needs the process list, the model and the categories, all of which live in the datacard configuration. So --config joins --input, --output and --era as a fixed argument the task always supplies, rather than something each analysis has to remember to add to args:. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
Reverts the dependency swap in 7bb81a5. That commit answered kandrosov's "since we now have PlotResonantLimitsTask, we should use it" by making this task require the plots, which pulled the limits in behind them. Reverting on the analysis's request: this task is for getting the limits together with the histograms that fed them, and the limit plots the configuration declares in `limit_plots` are worth asking for on their own, by running PlotResonantLimitsTask directly, rather than being dragged in by every request for limits. Keeps the get_eras() -> get_plottable_eras() rename from the same commit. That was a separate fix: the method returns the complement of ResonantLimitsTask.get_eras(), and two sibling tasks with a same-named method returning opposite sets is what made a configuration listing both the sub-eras and their group look like double counting. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
Review: use isinstance for the unc.type checks. The three call sites asked what an uncertainty *is* by comparing an enum its own subclasses derive: LnNUncertainty and MultiValueLnNUncertainty both report UncertaintyType.lnN, ShapeUncertainty reports shape. isinstance says the same thing against the class that already encodes it. The mapping is exhaustive -- those are the only three classes defining `.type`, and AutoUncertainty defines none, since resolveType turns it into one of them before either branch is reached -- so this is a rename, not a change of behaviour. `.type.name` stays where it is: AddSyst wants combine's string for the type, which is a name lookup rather than a test. Also fixes the shape plotting, which passed every configured category to HistPlotter even when the preprocessing had dropped one. At MX=300 the boosted categories are gated out (signal 0.13 < 0.5), HistPlotter exits 1 on the first key it cannot find, and the exception took the recovery panels and all three grids with it -- 12 plots written instead of 39, with only a warning. The keys are now filtered to what the summed shape file holds. Datacards were never affected: the MX=300 card has its 24 bins. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
It was added when the rebinning had just left the law chain and a hand-run script was the only way to invoke it. PreprocessShapesTask runs rebin_2d.py from the datacard configuration's `preprocess:` block now, so the supported way to rebin is the chain, and rebin_2d.py remains a standalone executable for anyone who wants to call it directly. Keeping it meant a second copy of the task's argument contract, which had already drifted -- its header described three arguments after --config made four -- plus a hardcoded production version that was only ever valid for one person, and a `cd ../..` that assumed StatInference sits inside an analysis laid out like HH_bbWW. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
Author
|
@cms-flaf-bot please test
|
|
pipeline#15642772 started |
addUncertainty converted every lnN under a meta-era into a shape. That is what makes a per-era lnN work at all -- lumi_13p6TeV_2022 covers two of Run3_Early's four sub-eras, and a single lnN on the summed yield cannot say that, so the varied shape is built sub-era by sub-era instead. An lnN with no era scoping needs none of it. QCD_scale_TT, PDF_alphas and top_mass apply uniformly to all four sub-eras, and converting them produced a perfectly flat template -- Up/nominal 1.025 in every bin, exactly the configured value, carrying no shape information -- while a standalone Run3_2022 card kept the same uncertainty as a true lnN. The two cards then modelled it differently: combine morphs a shape and treats an lnN as an exact log-normal. So the conversion is now gated on lnNIsEraDependent(). For the multi-value case that reads the eras off the value keys rather than unc.eras, because fromConfig reassigns its args dict inside the sub-entry loop and leaves the entry carrying the last sub-entry's scoping. Nothing reads those attributes today, but they are wrong, and a guard built on them would have classified lumi_1_13p6TeV by its 2023 half alone. Verified against the HH->bbWW configuration: both lumi entries and the prescribed lumi_1/lumi_2 pair classify era-dependent, while QCD_scale_*, PDF_alphas, top_mass and the BR uncertainties classify uniform and keep their lnN. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
PlotPullsAndImpacts is a dhi task, so its output stayed under inference/data/store while everything else in the chain is written to fs_default. This wraps it the way PlotResonantLimitsTask wraps the limit plots: an `impact_plots` block in the datacard configuration names the mass points and the plot parameters, and the finished PDFs are copied to <version>/ImpactPlots/<mass>/ with a manifest. The input already exists -- ResonantLimitsTask writes one combined card per mass, every era in it, and its shape paths are absolute, which is what makes it usable at all since PullsAndImpacts runs combine in a temporary directory. hh_model is pinned to NO_STR. PullsAndImpacts is a plain POITask, so it would otherwise default to the non-resonant model_default and fit r together with kl, kt, CV and C2V, building its own workspace under a hh_model__model_default/ path instead of the one the resonant chain already made. dhi's own resonant tasks pin it the same way. Three guardrails, each for something already seen on these cards: - mc_stats with parameters_per_page unset is refused. The combined cards carry a few hundred autoMCStats bins, and the default of -1 means one page, so the plot came out an unreadable hairline strip rather than an error. - after the fit, the merged JSON's parameters are diffed against the card's own nuisance lines. robustHesse drops what it cannot invert, logs it, and exits successfully, so a missing nuisance is invisible on the plot; on the Run3_Early cards this happens to CMS_res_j. - masses must be listed explicitly. This is a per-parameter workflow -- about two combine fits per nuisance per mass, so ~150 fits per mass at 76 nuisances -- which is also why it is its own task rather than something the limit chain drags in. plot_command, plot_targets and draw_plot move to DhiPlotMixin, shared with PlotResonantLimitsTask rather than copied: the "law exited cleanly but wrote no file" check matters here just as much, since luigi's retcode defaults return 0 on failure. Verified: law index writes 78 task families, the graph resolves PlotPullsAndImpactsTask -> ResonantLimitsTask -> CreateDatacardsTask, PlotResonantLimits- Task still resolves and keeps its redraw parameter, and the guardrails fire as intended. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_011t4wkv5deBQHyYkAf5KzHV
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
Author
|
@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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.