[HS3] Deduplicate HistFactory modifiers during HS3 export#22824
Merged
guitargeek merged 8 commits intoJul 16, 2026
Conversation
Test Results 23 files 23 suites 3d 11h 21m 5s ⏱️ For more details on these failures, see this check. Results for commit cdff5c1. |
guitargeek
force-pushed
the
fix/hs3-canonicalize-modifiers
branch
2 times, most recently
from
July 16, 2026 10:27
9568c75 to
393df87
Compare
Also fixes RooJSONFactoryWSTool::warning() to log at WARNING level instead of ERROR, and a self-comparison bug in the test validate() helper.
Extract the RooPoisson/RooGaussian constraint dispatch that was duplicated in the Barlow-Beeston (mc-stat) and shapesys branches of readChannel into a single constraintRelError() helper.
Replace the hand-rolled character-by-character number parser with a std::istringstream extraction that requires the whole string to be consumed. This mirrors the parsing done by toDouble(), so isNumber(s) is true exactly when toDouble(s) can convert the entire string. std::from_chars for floating-point types is not portably available on all platforms ROOT supports, so the stream extraction is used instead.
- Drop the pointless static_cast<RooSimultaneous*> around workspace.pdf() in importAnalysis: the result is stored as RooAbsPdf* anyway, and the cast is undefined behaviour when the pdf is not a RooSimultaneous. - Use dynamic_cast with a null check instead of an unchecked cast + deref in importDecay (resolutionModel) and importBinWidthFunction (histogram), so malformed input raises a clear error instead of crashing.
- Remove the file-global 'verbose' flag in JSONFactories_HistFactory that was never set to true, along with its two dead diagnostic blocks. - Remove the unused <mutex> include in JSONIO.cxx. - Remove an unused local and two commented-out lines.
Replace the verbose Doxygen blocks on the file-local (anonymous-namespace) helpers and the private member functions of RooJSONFactoryWSTool with concise one-line comments. Full Doxygen is kept only for the user-facing public API declared in RooJSONFactoryWSTool.h. Also move the exportCategory() doc block, which had become detached from its function, back onto it.
guitargeek
force-pushed
the
fix/hs3-canonicalize-modifiers
branch
from
July 16, 2026 10:32
393df87 to
fa04120
Compare
guitargeek
approved these changes
Jul 16, 2026
guitargeek
left a comment
guitargeek
left a comment
Contributor
There was a problem hiding this comment.
LGTM!
I have also added a few cleanup commits at the same time, to ensure the codebase doesn't grow too much.
Contributor
|
/backport to 6.40 |
|
Preparing to backport PR #22824 to branch 6.40 requested by guitargeek |
|
This PR has been backported to branch 6.40: #22836 |
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
3 participants
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.
Motivation
RooFit allows a reconstructed HistFactory sample to contain multiple modifiers with the same name and type. HS3, however, requires modifiers within a sample to be unique by
(name, type).Previously, the HistFactory exporter could therefore produce invalid HS3 when serializing such RooFit workspaces.
Changes
This PR canonicalizes HistFactory modifiers before serializing a channel:
Duplicate
normsysmodifiers are combined by multiplying theirloandhianchors.Duplicate
histosysmodifiers are combined additively around the nominal histogram:Compatible duplicates are exported as a single modifier.
A RooFit
WARNING/IOmessage is emitted for every combined group, identifying the channel, sample, modifier name and type, and number of duplicates.Shared constraints are queued for export only once.
Modifier ordering remains deterministic.
Duplicates are considered compatible only if their parameter, interpolation code, constraint metadata, and—where applicable—histogram binning agree. Incompatible duplicates produce a descriptive RooFit
ERROR/IOmessage and throw, preventing the HistFactory object from being exported.A final uniqueness check rejects any remaining duplicate modifier type that cannot be represented safely as a standard HS3 modifier.
Additional fixes
RooJSONFactoryWSTool::warning()to log atRooFit::WARNINGrather thanRooFit::ERROR.Tests
The new tests cover:
normsysandhistosysmodifiers.normsysanchors.histosysdeviations around the nominal histogram.WARNINGlevel.The complete
testRooFitHS3test suite passes. The motivating workspace was also successfully exported with no remaining duplicate(name, type)modifier groups.Interpolation note
For code-4
normsysmodifiers, multiplying theloandhianchors preserves the combined response at nuisance values-1,0, and+1. A single code-4 interpolation cannot, in general, exactly reproduce the product of multiple interpolations between those anchors.@cburgard @stalbrec