Repository navigation
[RF][HS3] Bring the HS3 import and export closer to the HS3 standard - #23996
Merged
guitargeek merged 35 commits intoNov 5, 2026
Merged
Conversation
The importer only checked that `metadata.hs3_version` exists. It now also requires it to be a string, parses the leading `MAJOR.MINOR`, and warns if the document is newer than the version RooFit writes (or if the string cannot be parsed). The exported version remains a single constant. The parsed version is kept available to importers during the import via `RooJSONFactoryWSTool::hs3VersionInput()`. Assisted-by: claude-code:claude-opus-5-5
HS3 requires the names of all objects in a document to be unique. The importer built its function/distribution indices with `emplace`, so a duplicate silently resolved to the first definition. Duplicated names within or across `distributions`, `functions`, `data`, `domains` and `parameter_points` are now an error. RooFit itself named likelihoods after their analysis, and analyses without matching dataset after the pdf, so its own files violate this rule. On export, likelihoods are now called `<analysis>_likelihood`, and an analysis without dataset is named after its ModelConfig. On import, a name clash that involves `likelihoods` or `analyses` only triggers a warning, so that files written by older ROOT versions can still be read. This is unambiguous: analyses are not referenced by name, and likelihoods are only looked up from analyses. Assisted-by: claude-code:claude-opus-5-5
The importer required a domain named exactly `default_domain`, which is not part of the HS3 standard. If it is absent, variables are now created with the range of the first domain that mentions them (or RooFit's default if none does). Export still writes `default_domain`, and documents that contain it are imported exactly as before. Assisted-by: claude-code:claude-opus-5-5
cburgard
requested review from
dpiparo,
guitargeek,
hageboeck and
silverweed
as code owners
November 5, 2026 10:01
2 tasks done
Contributor
Author
|
Hey @guitargeek , this PR factors out a few of the upcoming changes to the HS3 import/export that will be needed for HS3 milestone 0.3. |
cburgard
force-pushed
the
hs3-conformance-fixes-independent
branch
from
November 5, 2026 12:35
ce4ffc6 to
2b083bb
Compare
A parameter listed in a parameter point that was not created by any distribution, function or domain used to be ignored silently. It is now created as a `RooRealVar`. It is constant if `"const": true` or if no domain lists it, since there is no range to vary it in; `"const": false` is the same as omitting `const`. A string `value` is accepted for categorical parameters if a `RooCategory` of that name exists; otherwise it is ignored with a warning instead of being read as a number. Assisted-by: claude-code:claude-opus-5-5
HS3 forbids `nbins` and `edges` on the axes of unbinned data, but the unbinned-data exporter used the shared binning writer. It now writes only `name`, `min` and `max` for each axis. The binning of the observables is still preserved in the domains. Assisted-by: claude-code:claude-opus-5-5
On export, if a ModelConfig has a snapshot, the analysis now references the parameter point of that snapshot as `init`. On import, the snapshot is re-created under the same name so that repeated round trips are stable, and an `init` that refers to a parameter point that doesn't exist is reported with a warning instead of being skipped silently. (All parameter points are imported before the analyses, so their order in the file does not matter.) Assisted-by: claude-code:claude-opus-5-5
* RooArgusBG is exported as `argus_dist`. `ARGUS_dist` is still imported, with a one-time deprecation warning. * RooBinSamplingPdf is exported as `binsampling_dist`, the key its importer was registered for, so it can be read back. `binsampling` is still imported, with a one-time deprecation warning. * `rate_extended_dist` uses the HS3 fields `rate` and `dist`. `norm` and `pdf` are still accepted on import. RooFormulaVar and RooGenericPdf were already exported as `generic` and `generic_dist`, with `generic_function` still importable, so they needed no change. The Crystal Ball keys are left unchanged here; they follow a pending change of the HS3 standard. Assisted-by: claude-code:claude-opus-5-5
HS3 expresses conditionals as `ifelse(condition, a, b)`, but RooFit wrote the C-style ternary `c ? a : b` that TFormula uses. The generic formula exporter now rewrites ternaries at any nesting level into `ifelse()`. If an expression can't be rewritten safely, a warning is printed and the expression is exported unchanged. TFormula doesn't understand `ifelse()`, so on import it is translated back into a parenthesized ternary. Without this, RooFit could not read its own output. Expressions without ternaries are exported unchanged. Assisted-by: claude-code:claude-opus-5-5
The `extended` field of `mixture_dist` was ignored on import; the RooAddPdf derived its extended mode from the number of coefficients only. If the flag is present, it is now checked against the coefficient count: * `extended: true` with one coefficient fewer than summands is an error (HS3 requires such a mixture to be non-extended); * `extended: false` without coefficients is an error (RooFit would build an extended sum of extended distributions); * `extended: false` with one coefficient per summand is valid HS3, but a RooAddPdf cannot be forced to be non-extendable, so a warning advises not to use the extended term when fitting. Assisted-by: claude-code:claude-opus-5-5
…factors * HS3 spells the log-normal constraint type `LogNormal`. It is now recognized as a constraint type like the older RooFit spelling `Lognormal`, which is kept as an alias. * For a `normfactor` that has both `name` and `parameter`, the RooRealVar was named after `name`, breaking the correlation of normfactors that share a `parameter` but have different labels. The parameter is now named after `parameter`; `name` is only the label of the modifier. Assisted-by: claude-code:claude-opus-5-5
…trained In HS3, a modifier without `constraint` is unconstrained. RooFit instead created a unit Gaussian constraint for `normsys` and `histosys` modifiers that had none of `constraint`, `constraint_name` or `constraint_type`. Now no constraint is created and an info message is printed. The `nom_*` global observable is only created for constrained modifiers. On export, a non-constant normsys/histosys parameter without a recognized constraint used to be an error. Such a modifier is now exported without `constraint`, as long as no other constraint term of the channel depends on its parameter (that case is still an error, because the constraint is not supported). This keeps RooFit able to write the models it reads. The rf515_hfJSON tutorial relied on the implicit Gaussian constraints; its normsys modifiers now request them with `"constraint": "Gauss"`, so the tutorial model is unchanged. For the same reason, the "Constraint" case of the HistFactoryIncompatibleDuplicateModifiers test now requests its two different constraint terms explicitly. Assisted-by: claude-code:claude-opus-5-5
… modifier HS3 gives the constraint type of a `staterror` on the modifier (`Poisson` or `Gauss`), but RooFit ignored it and only read the private channel-level field `histfactory_dist.staterror.constraint_type`, defaulting to Poisson. The export wrote neither, so the choice between Gaussian and Poisson stat constraints was lost. On import, the `constraint` of the staterror modifier is now used, with the channel-level field as fallback. On export, the constraint type is written both on the staterror modifiers and on the channel level, so that the files can still be read by older ROOT versions. Assisted-by: claude-code:claude-opus-5-5
…>= 0.3 HS3 requires `default_interpolation` if a modifier has no `interpolation`, and forbids implementation-specific defaults. RooFit silently fell back to code 4. Documents with `hs3_version` 0.3 or newer that rely on this are now rejected. ROOT 6.38 and earlier never wrote the interpolation of histosys modifiers (or of normsys modifiers with the default code 4), nor a `default_interpolation`. To keep these files readable, documents with an older `hs3_version` (and objects imported without a document, e.g. with `importJSONElement()`) still get code 4, with one deprecation warning per channel. The export already writes `default_interpolation` whenever a modifier omits `interpolation`, so RooFit doesn't produce such documents. The rf515_hfJSON tutorial now states its interpolation explicitly with a `default_interpolation`, which avoids the deprecation warning. Assisted-by: claude-code:claude-opus-5-5
Malformed input could be read out of bounds: * the rows of `multivariate_normal_dist` covariance and correlation matrices are now checked to form a square matrix of the right size; * the `errors` of binned data must have as many entries as `contents`; * the `edges` of binned axes must contain at least two strictly ascending values. Assisted-by: claude-code:claude-opus-5-5
The `min`/`max` of the axes of unbinned data were ignored on import; the observables kept whatever range they had, and entries outside of it were clipped. If an observable got no range from a domain, its range is now set from the axis. If a domain range disagrees with the axis, it is kept and a warning is printed. Assisted-by: claude-code:claude-opus-5-5
* The export keys and factory expressions are no longer central JSON files in `etc/`, but catalogs embedded in `roofit/hs3/src/RooFitHS3_wsexportkeys.cxx` and `roofit/hs3/src/RooFitHS3_wsfactoryexpressions.cxx`. Point to them, and explain how to add entries there or load additional catalogs at runtime. * Describe `importArg()` as the function to override, with `importFunction()`/`importPdf()` as compatibility shims. * Register importers and exporters with the template helpers or with `std::unique_ptr`, as the current API requires, instead of raw `new`. Assisted-by: claude-code:claude-opus-5-5
…e error RooDataHist only starts to store squared weights when a bin is set with a positive error, and then initializes them from the weights of the bins set so far. So for "errors": [0, 2], the first bin got a squared weight equal to its content instead of 0. The bin with the first positive error is now set first. Assisted-by: claude-code:claude-opus-5-5
cburgard
force-pushed
the
hs3-conformance-fixes-independent
branch
from
November 5, 2026 12:45
2b083bb to
5092e2b
Compare
…nstVar A RooRealVar that is constant in all parameter points and a RooConstVar are written the same way in the parameter points (a value and "const": true). Only the domains tell them apart: the RooRealVar is listed in a domain, the RooConstVar is not, and a parameter without domain is constant and read back as a RooConstVar. The test checks this, the restored types and constness (also from snapshots), and that the round trip is stable. Assisted-by: claude-code:claude-opus-5-5
The code-formatting check also covers JSON files, and the tutorial JSON changed in this PR didn't follow the clang-format style. Whitespace only: the parsed JSON content is unchanged. Assisted-by: claude-code:claude-opus-5-5
This was referenced Nov 5, 2026
Pdfs that share a parameter, like two RooExponentials with one slope, each wrote the same helper function. The duplicate-name check then rejected the file on import. Assisted-by: claude-code:claude-opus-5-5
ModelConfigs sharing a pdf and a dataset, like the usual S+B and B-only pair, got the same analysis name. Only the last one survived, and its domains were written twice, which the duplicate-name check rejects. Assisted-by: claude-code:claude-opus-5-5
RooFit still writes clashing names, for example for a named range and a pdf of the same name, and it labels its files as older than 0.3. Throwing on these made it unable to read its own output, so they only get a warning now, like a missing interpolation. Assisted-by: claude-code:claude-opus-5-5
The exporter writes infinite bounds as null, but the new range handling for unbinned axes called val_double() on them. Files with an unbounded observable could not be read anymore. Assisted-by: claude-code:claude-opus-5-5
Zero coefficients for one summand also satisfied the check for "one coefficient fewer than summands", so RooFit rejected its own export of a RooAddPdf with a single extended pdf. Assisted-by: claude-code:claude-opus-5-5
parseOverallModifierFormula() requires a constraint but didn't store it, which only worked because the importer implied a Gaussian. Now that a missing constraint means unconstrained, the export threw instead, claiming that the RooGaussian is not supported. Assisted-by: claude-code:claude-opus-5-5
A formula like 1+0.1*alpha was exported with hi=0.1 and lo=-0.1, but the factors of a normsys are absolute: hi=1.1 and lo=0.9. The re-imported model scaled the sample by 0.1 at alpha=1, and evaluated to NaN for alpha<0. Assisted-by: claude-code:claude-opus-5-5
GetSnapshot() was only called as a null check, but it loads the snapshot and restores nothing but the values. After exportJSON(), a fixed parameter was floating and had lost its error and step width. Scanning the snapshot names also dropped or mixed up "init" for ModelConfigs with a common name prefix, like ModelConfig and ModelConfig_with_poi_0, and after several SetSnapshot() calls. Assisted-by: claude-code:claude-opus-5-5
AddLogNormSyst() and AddGammaSyst() of HistFactory::Measurement use a function like alphaOfBeta_x as normsys parameter. The check for unsupported constraints missed this case, so it was exported as a free parameter and the re-imported model silently differed. Assisted-by: claude-code:claude-opus-5-5
The former HistFactory::JSONTool and the old rf515 tutorial file write normsys and histosys modifiers without constraint and with version 0.1.90. These models rely on the implicit unit Gaussian, so dropping it silently changed their fits. Objects imported without a document also keep it. Assisted-by: claude-code:claude-opus-5-5
Older RooFit versions constrained these parameters, so a floating one might not be what the author of the document meant. Warn once per channel instead of writing an info message. Assisted-by: claude-code:claude-opus-5-5
createPHF() turned everything except "Poisson" and "Const" into a Gaussian. A shapesys with "constraint": "LogNormal", the HS3 spelling, therefore silently got the wrong constraint instead of an error. The older spelling "Lognormal" did the same. Assisted-by: claude-code:claude-opus-5-5
ROOT 6.38 and 6.40.00 wrote this key instead of "constraint", so their Gaussian stat errors still came back as Poisson. Assisted-by: claude-code:claude-opus-5-5
Without any positive error, RooDataHist never started to store squared weights, so "errors": [0, 0] still read back as squared weight = content while [0, 2] was fixed. Switch on the squared weights up front instead of setting the first bin with a positive error first. Assisted-by: claude-code:claude-opus-5-5
The index categories of combined distributions were created after the parameter points were read. A string value for such a category was dropped, and a numeric value created a RooRealVar that took the place of the category. Assisted-by: claude-code:claude-opus-5-5
RooJSONFactoryWSTool::warning() logged on RooFit::IO, which no default stream covers. Users therefore never saw these warnings, for example about newer HS3 versions, deprecated type keys, name clashes or lost constraints. The tests only did because they hijack the stream. Assisted-by: claude-code:claude-opus-5-5
guitargeek
approved these changes
Nov 5, 2026
guitargeek
changed the title
This was referenced Nov 5, 2026
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.
This Pull request:
Brings RooFit's HS3 import/export closer to the HS3 standard (
hep-statistics-serialization-standardmasteratcb3e9eb) with small, independent fixes: renamed type keys, added validation and warnings, andchanged defaults. All changes follow the HS3 standard as it is merged today; none depend on pending HS3 PRs.
These are the first 19 commits of #23840 ("Group I"), split
off so that they can be merged now. #23840 stays in draft with the parts that follow
pending HS3 PRs (#135, #124), and will be rebased on this PR.
Background: the HS3–RooFit compatibility audit (October 2026)
of RooFit's HS3 layer against HS3
cb3e9eb(findings also tracked in HS3 issues #95 "Inconsistencies between ROOT HS3 export and spec",#99 "Potential spec deviations in HS3TestSuite" and #133 "Checklist: RooFit HS3
types/fields not yet in the standard"). Related ROOT PR: #23839
(pinned HS3TestSuite with an expected-failure list).
Each commit builds on its own and passes
testRooFitHS3,testJSONInterfaceandtestHistFactory.Pre-flight findings
Behaviour on
masterbefore this PR:exportChannel()inJSONFactories_HistFactory.cxx): RooFitwrites
"constraint": "<name of the constraint distribution>"(e.g."alpha_syst1Constraint") and exports thatdistribution separately. It does not write
constraint_name/constraint_type(ROOT 6.38 still wroteconstraint_name). On import,constraintis first resolved as a distribution name and then as a legacy type(
Gauss, ...). This PR keeps this and only adds thestaterrorconstraint type of the current standard.Changes or fixes:
RooJSONFactoryWSTool.cxx,JSONIOUtils.cxxhs3_versionmust be a string. ParseMAJOR.MINOR, and warn if it is newer than the supported version or can't be parsed. The export value stays a single constant. The version of the document being imported is available viaRooJSONFactoryWSTool::hs3VersionInput().VersionStringHandlingRooJSONFactoryWSTool.cxxdistributions,functions,data,domains,parameter_points. A clash involvinglikelihoodsoranalysesonly warns (legacy ROOT files, see below). Export: likelihoods are named<analysis>_likelihood, and an analysis without dataset is named after its ModelConfig.DuplicateNamesAreRejecteddefault_domainoptionalDomains.cxxdefault_domain, variables take the range of the first domain that mentions them. Still written on export.DefaultDomainIsOptionalRooJSONFactoryWSTool.cxxRooRealVar. It is constant if"const": trueor if no domain lists it (there is no range to vary it in);"const": falseis the same as omittingconst. This is valid under the current standard (such a parameter must be listed in a domain or beconst) and matches HS3 PR #131. Stringvaluesets aRooCategoryof that name; otherwise it is ignored with a warning.ParametersOnlyInParameterPointsRooJSONFactoryWSTool.cxxname/min/max.UnbinnedDatasetAxesHaveNoBinninginitin analysesRooJSONFactoryWSTool.cxxanalyses[].init. Import re-creates it under the same name (stable round trips), and warns if the parameter point doesn't exist.AnalysisInitRoundTripJSONFactories_RooFitCore.cxx,RooFitHS3_wsexportkeys.cxx,RooFitHS3_wsfactoryexpressions.cxxargus_dist(ARGUS_distdeprecated alias);binsampling_distfor export and import (binsamplingdeprecated alias);rate_extended_distusesrate/dist(norm/pdfaccepted).generic/generic_distwere already conformant. The Crystal Ball keys are not changed here: they follow a pending change of HS3 PR #135, see #23840.TypeKeysMatchTheCatalogifelseJSONFactories_RooFitCore.cxxc ? a : b(nested, in parentheses) toifelse(c,a,b); it warns and leaves the expression unchanged if that isn't safely possible. Also translatesifelseback on import, because TFormula doesn't understand it.TernaryIsExportedAsIfElsemixture_dist.extendedJSONFactories_RooFitCore.cxxextendedagainst the coefficient count; throw on contradictions.MixtureDistExtendedFlagLogNormal, normfactorparameterJSONFactories_HistFactory.cxxLogNormal(keepLognormal). The normfactor parameter is named afterparameter;nameis the label only.HistFactoryNormFactorParameterAndLogNormalJSONFactories_HistFactory.cxxnormsys/histosyswithout a constraint key are unconstrained (info message). Export accepts unconstrained modifiers instead of throwing "cannot find constraint".HistFactoryModifierWithoutConstraintIsUnconstrainedstaterrorconstraintJSONFactories_HistFactory.cxxconstraint(Poisson/Gauss) from the staterror modifier, falling back to the channel-levelstaterror.constraint_type. Export writes both. (To be superseded on export by the named constraints of HS3 PR #124, see #23840.)HistFactoryStatErrorConstraintJSONFactories_HistFactory.cxxinterpolationand there is nodefault_interpolation, forhs3_version≥ 0.3. Older documents keep code 4, with one deprecation warning per channel. Export already always writesdefault_interpolationwhen needed (verified, unchanged).HistFactoryMissingInterpolationJSONFactories_RooFitCore.cxx,RooJSONFactoryWSTool.cxx,JSONIOUtils.cxxerrorslength =contentslength;edgeshas ≥ 2 strictly ascending values.MalformedArraysAreRejectedRooJSONFactoryWSTool.cxxmin/maxfrom the axis; a disagreeing domain range is kept with a warning.UnbinnedAxisRangesOnImportroofit/doc/developers/roofit_hs3.mdetc/*.json;importArg()as primary API;std::unique_ptr/ template registration. The snippets were compile-checked against the current headers.RooJSONFactoryWSTool.cxxBinnedDataZeroErrorsRooConstVarvs. constantRooRealVarRooRealVarthat is constant in all parameter points and aRooConstVarare written the same way in the parameter points (a value and"const": true); only the domains tell them apart. The new test checks this, the restored types and constness (also from snapshots), and that the round trip is stable.ConstRooRealVarStaysDistinctFromRooConstVartutorials/roofit/roofit/rf515_hfJSON.jsonDeviations from the original plan, and the reasons:
pdf. Throwing on these would make every file written by an existing ROOT release unreadable. Analyses aren't
referenced by name, and likelihoods are only looked up from analyses, so these clashes only warn. New exports
don't produce them.
ifelse→ ternary translation on import was added, since otherwise RooFit couldn't read its ownoutput.
RooAddPdfcan't be forced to be non-extendable.extended: falsewith one coefficient per summand(valid HS3) therefore only warns; the shape is the same.
histosysmodifiers (or ofnormsysmodifiers withcode 4), nor
default_interpolation. Throwing unconditionally would make all HistFactory JSON from released ROOTversions unreadable, so the strict behaviour is gated on
hs3_version≥ 0.3. Objects imported without adocument (
importJSONElement) are treated as legacy.combined-data path), so no
categoriesbranch was added.Behaviour changes users may notice
normsys/histosyswithoutconstraintare no longer silently Gaussian-constrained. Hand-written filesthat relied on this must add
"constraint": "Gauss". Therf515_hfJSONtutorial JSON was updated accordingly;its fit result is identical to
master(min NLL −1043.45938119, same parameters).hs3_version≥ 0.3. Older documents get a deprecationwarning and the old code-4 default.
ARGUS_dist→argus_dist,binsampling→binsampling_dist,rate_extended_distfieldspdf/norm→dist/rate.<analysis>_likelihood; analyses without dataset are named after theModelConfig.
errorsor invalidedgesare now rejected.ifelse()instead of?:.bin content as squared weight).
Changed tests and reference data
No reference JSON files exist for these tests; expectations are inline. The following existing expectations or
fixtures were changed because of intended behaviour changes:
HistFactoryIncompatibleDuplicateModifiers/Constraint: the two modifiers now request"constraint": "Gauss"explicitly, because the test needs two different constraint terms (previously these were implicit).
tutorials/roofit/roofit/rf515_hfJSON.json: explicit"constraint": "Gauss"on the threenormsys, and adefault_interpolation.HistFactory round trip
roofit/histfactory/test/ref_6.16_example_UsingC_combined_meas_model.root(workspacecombined), exported withmasterand with this branch. Every changed key:analyses[simPdf_obsData].likelihood:"simPdf_obsData"→"simPdf_obsData_likelihood"(same forsimPdf_asimovData)likelihoods[simPdf_obsData]→likelihoods[simPdf_obsData_likelihood](same forsimPdf_asimovData),content unchanged
distributions[model_channel1].samples[background1_…].modifiers[staterror].constraint: absent →"Poisson"(same for
background2_…)distributions[model_channel1].staterror.constraint_type: absent →"Poisson"A second export after re-importing differs from the first in the same keys on
masterand on this branch(legacy
binWidth_*variables of the 6.16 workspace,misc.minimization.parameter_stepwidths). That ispre-existing and not touched here.
Checklist:
Tests run (Linux, Ubuntu 26.04, GCC 15.2, RelWithDebInfo,
-Droofit=ON -Dtesting=ON):ctest -R 'RooFitHS3|JSONInterface': baseline 71 + 1 tests pass; with this PR 88 + 1 tests pass (17 new). Thetwo test binaries and
testHistFactory(69 tests) were also built and run for every single commit.Not run: the PyROOT tests (
bindings/pyroot/pythonizations/test/roofit.py, which uses HistFactory JSON viaimportJSONElement), therf515_hfJSON.pytutorial itself (its model was fitted from C++ instead), and theoptional HS3 test suite (
test_roofit_hs3testsuite). My build haspyroot=OFF.AI assistance: Claude Code (claude-opus-5-5) implemented the changes, the tests and this description from a
written plan, under review of the author. The commits carry
Assisted-by: claude-code:claude-opus-5-5.🤖 Generated with Claude Code