Repository navigation
[RF][HS3] Import and export through a typed representation of HS3 documents - #24107
Merged
Merged
Conversation
Contributor
|
That's a pretty impressive refactor, nice work! |
Test Results 22 files 22 suites 3d 12h 59m 53s ⏱️ For more details on these failures, see this check. Results for commit ce3c956. ♻️ This comment has been updated with latest results. |
Contributor
Author
|
Yes, I intend to merge this today because I think the new types and other added guardrails like the check that RooFit understands the whole JSON document with warnings if it doesn't are pretty useful also for your PRs! And your PRs are mostly touching the RooFit-dependent side I think, which is not changes as much. |
guitargeek
force-pushed
the
hs3-document
branch
2 times, most recently
from
November 12, 2026 12:37
dc6f49e to
d8bb5a2
Compare
Contributor
Author
|
Rebased on top of master to fix the conflicts with #24099 |
Objects that depend on each other made the import recurse until the stack overflowed. Keep track of the objects that are being imported and report the cycle instead. Assisted-by: claude-code:claude-opus-5-5
An analysis that listed an unknown parameter of interest dereferenced a null pointer. Assisted-by: claude-code:claude-opus-5-5
Parsing and building RooFit objects are intertwined, so that malformed documents are only noticed halfway through an import, and every importer deals with JSON details on its own. Add RooFit::HS3Document, which follows the structure of the standard and doesn't depend on RooFit, and a parser that reports all problems with their location in the document. The header stays private for now. Distributions and functions are generic objects for the time being, which are still interpreted by the registered importers. Assisted-by: claude-code:claude-opus-5-5
Parse the whole document before anything is created in the workspace, and use the typed representation for all top-level sections. Most of the import state moves out of the public header. The exported JSON doesn't change. Assisted-by: claude-code:claude-opus-5-5
Parse HistFactory distributions into typed modifiers with a variant for the constraint, and create all constraints in one place. What older RooFit versions wrote differently, like integer interpolation codes, "constraint_type", or implied Gaussian constraints, is rewritten into the current form before parsing, for which JSONNode gets erase(). The builder has no version checks anymore. This fixes some imports that crashed or built the wrong model: - a "Gauss" constraint of a normfactor, whose nominal value is one - a sample without "modifiers", which lost its histogram - a shapesys or shapefactor with a single "parameter", as in the example of the standard, and a shapefactor without data - modifiers without name in different samples, which shared objects because they got the same generated name - a custom modifier that refers to an object later in the document Objects of different samples or modifiers that would get the same name are reported instead of being shared. A function as parameter of a modifier is reported instead of corrupting memory, and a constant without domain is no longer used as a variable, which was undefined behaviour. Importers that users register for "histfactory_dist" still have priority. The exported JSON doesn't change. Assisted-by: claude-code:claude-opus-5-5
A component that RooFit doesn't know, like a misspelled key, was silently ignored. Report it with its location, also for the objects that are imported by the registered importers: the JSON tree tells a JSONReadRecorder about each read of a map. Older versions of RooFit wrote the binning of the axes of unbinned data, which is now removed before parsing. Assisted-by: claude-code:claude-opus-5-5
Check what doesn't depend on how the objects are built before anything is created in the workspace: that names are unique, also with respect to the parameters, that references can be resolved, that binnings are valid, and that the sizes of arrays are consistent. Documents that fail these checks are not imported. Assisted-by: claude-code:claude-opus-5-5
Collect everything but the generic objects, which the exporters still write directly, in an HS3Document, check it with the same validation as on import, and write it in one place. HistFactory distributions are exported as typed modifiers. The exported JSON doesn't change, except that an empty workspace now gets an empty array of default parameter values instead of null, and no "domains": null. Assisted-by: claude-code:claude-opus-5-5
Export RooGaussModel as "gauss_model_function" and RooTruthModel as "truth_model_function", which the standard lists as functions, and RooAddModel as "mixture_model". The former keys are still read, with a deprecation warning. Assisted-by: claude-code:claude-opus-5-5
The standard requires them for all continuous axes of binned data, also if they have "edges". Bounds that don't agree with the edges are now rejected, like axes with both "edges" and "nbins". Older documents without them are still read. Generic formulas accept "min" and "max" next to "edges", and write them like the other binned axes. Assisted-by: claude-code:claude-opus-5-5
Map the "prior" of an analysis to the prior pdf of the ModelConfig. Assisted-by: claude-code:claude-opus-5-5
Constraint terms of a channel that no modifier names in its "constraints" were lost with a warning on export, and the export failed if they depended on the parameter of a modifier. They are now exported as "aux_distributions" of the likelihoods, which the standard adds to the constraint terms of the prediction, and recorded in misc.ROOT_internal.histfactory_constraints, which puts them back into the channel on import. A Gaussian of a normfactor parameter that is not a term of the channel, like an external constraint or a prior, is no longer exported as the constraint of the modifier. Assisted-by: claude-code:claude-opus-5-5
The correlations of the uncertainties of binned data and the uncertainties of the entries of unbinned data were dropped silently, because RooDataHist and RooDataSet can't store them. Datasets of type "point" get a clear error. Assisted-by: claude-code:claude-opus-5-5
The document can't tell what RooFit makes of its objects. Parameters of interest that name another object, or that are constants because no domain lists them, and likelihood terms, priors, auxiliary distributions or components of combined distributions that are functions in RooFit or skipped made the import crash or were dropped silently. They are reported now. Assisted-by: claude-code:claude-opus-5-5
Only the tool uses it, and its header forward-declares it for the _domains member already. Assisted-by: claude-code:claude-opus-5-5
The ranges of the variables are now looked up with HS3::DomainRanges, which doesn't depend on RooFit, and the domains are written directly as HS3::Domain. Variables are created in the order of the document. Assisted-by: claude-code:claude-opus-5-5
The exporter fills an HS3::HistFactoryDist instead of its own channel model. Merging duplicate modifiers and choosing the default interpolation now work on the document, so they don't depend on RooFit. Inconsistent binnings are reported by the validation of the exported document. Assisted-by: claude-code:claude-opus-5-5
The functions "interpolation" and "interpolation0d" are read into HS3::InterpolatedFunction and HS3::InterpolatedNumber, which also handle the legacy "interpolationCodes", and written from them. The upgrade of legacy interpolation codes of modifiers shares the code. Assisted-by: claude-code:claude-opus-5-5
- Import a single variable without writing and parsing a document for it. - Read the axes of histograms with the parser instead of a second reader. - Drop the branches for useListsInsteadOfDicts, which is always true. - Check the size of channel-level staterror constraints in the validation instead of when building the model. Assisted-by: claude-code:claude-opus-5-5
There is no Writer that shares the describe() functions. Most structs are read with legacy keys and defaults that the writer doesn't mirror, so sharing them would not save code. Assisted-by: claude-code:claude-opus-5-5
guitargeek
force-pushed
the
hs3-document
branch
from
November 12, 2026 12:58
d8bb5a2 to
ce3c956
Compare
dpiparo
approved these changes
Nov 12, 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.
Makes RooFitHS3 read and write HS3 documents through
RooFit::HS3Document, a typed representation that follows the structure of the standard and does not depend on RooFit.Until now, parsing and building RooFit objects were intertwined: malformed documents were only noticed halfway through an import, and every importer dealt with JSON details on its own. With this PR a document is parsed and validated completely before anything is created in the workspace, and all problems are reported with their location in the document. The export collects everything except the various pdf and function objects in the same representation, checks it with the same validation, and writes it in one place.
In the future, we could even think about extending the strongly typed internal representation also to to the pdfs and functions, but that is currently not possible because of the open-world custom importers and exporters. Maybe that can change one day if HS3 is general enough so no custom IO is needed, because making the JSON IO of RooFit a closed-world system has clear benefits too.
The changes in this PR were fully implemented by AI (Claude Code with Opus 5.5), instructed with the architecture I had in mind.
Not adding any tests was a deliberate choice of mine: the tests that I'd write would only lock in current behavior which might or might not yet be fully HS3 conform, therefore acting as an additional source of truth that has to be taken into account while RooFit HS3 is still evolving. That's why I leave the implementation of tests (mostly regression and compatibility tests) to the developers of HS3 who are users at the same time, like @cburgard, @Phmonski, and @stalbrec.