[RF][HS3] Preserve explicit binnings for RooGenericPdf and RooFormulaVar#22842
Open
Phmonski wants to merge 4 commits into
Open
[RF][HS3] Preserve explicit binnings for RooGenericPdf and RooFormulaVar#22842Phmonski wants to merge 4 commits into
Phmonski wants to merge 4 commits into
Conversation
Test Results 23 files 23 suites 3d 15h 30m 48s ⏱️ Results for commit c63e052. |
The axis parser for RooGeneric/RooFormulaVar binning read "edges", "min" and "max" via JSONNode::val_double() without first checking the node was actually a number. For a non-numeric JSON value (e.g. a string) this either threw a backend-specific exception that is not a std::runtime_error (aborting past the tool's error handling) or, with the stringstream-based backend, silently coerced the value to 0 and imported a wrong binning. Add a JSONNode::is_number() predicate (native in the nlohmann backend, with a val()-parsing fallback in the base interface) and guard the edge and min/max conversions with it, so malformed bounds are rejected with a descriptive RooJSONFactoryWSTool::error() like every other malformed axis case. Full precision is preserved because the value is still read through val_double() once validated.
readPositiveInteger parsed "nbins" with a strict std::from_chars over the node's textual value, which rejected an integer bin count encoded as a JSON float: e.g. 1000000.0 renders as "1e+06", where from_chars stops at 'e' and the whole import fails with a misleading "must be a positive integer" error. Other nbins readers in HS3 use the lenient val_int(). Read the value through val_double() instead and require it to be finite, >= 1, integral and within int range. This accepts integer-valued floats like 1e6 while still rejecting fractional (2.5), non-positive (0) and non-numeric values, so the existing rejection tests keep passing. Drops the now-unused <charconv> include.
The separate roofit/jsoninterface package made sense when there was also a YAML backend, but since only the nlohmann/json backend remains, it was just unnecessary abstraction boilerplate as a standalone library. Move the RooFit::Detail::JSONInterface header into RooFitHS3 and merge the implementation (JSONInterface.cxx, JSONParser.h, JSONParser.cxx) into a single translation unit, with the TJSONTree implementation class now in an anonymous namespace. This remains the only translation unit that includes nlohmann/json.hpp, so the JSON engine could still be swapped out in the future by changing only this one file. Also remove the unused writeYML() interface method, a leftover from the YAML backend, and move testJSONInterface into the RooFitHS3 test suite.
guitargeek
force-pushed
the
feat/hs3-generic-binning
branch
from
July 22, 2026 08:00
c63e052 to
775655a
Compare
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.
Motivation
Following the introduction of explicitly binned
RooGenericPdfandRooFormulaVarobjects in #22708, their declared binning information was not preserved by HS3 serialization.Only the formula expression was exported, so an HS3 round trip lost the binning registered through
setBinning(). The imported object consequently no longer reported itself as a binned distribution and could not use the intended binned integration path.Changes
This PR adds optional inline
axeskey to HS3 generic formulas.Uniform binnings are represented as:
Non-uniform binnings use explicit edges:
The implementation:
getBinning()accessor toRooGenericPdfandRooFormulaVar.setBinning().binBoundaries()andplotSamplingHint()to use this accessor.axesonly when an explicit formula binning exists.setBinning(..., false)during import because serialized binnings are treated as trusted declarations, avoiding potentially expensive flatness sampling.RooFormulaVarexport type asgenericwhile continuing to import bothgenericandgeneric_function.No persistent members or class versions are changed.
JSONFactories_HistFactory.cxxand a few unrelated test lines contain formatting-only changes.Validation
Imported axes are checked for:
RooAbsRealLValuedependency.nbins.Tests cover:
RooGenericPdfround trips.RooFormulaVarround trips.The complete
GenericPdftest suite andtestRooFitHS3pass.@cburgard @will-cern