Skip to content

[RF][HS3] Add structured interpolation support for HistFactory modifiers - #22844

Open
Phmonski wants to merge 6 commits into
root-project:masterfrom
Phmonski:feat/hs3-export-interpolation-information
Open

[RF][HS3] Add structured interpolation support for HistFactory modifiers#22844
Phmonski wants to merge 6 commits into
root-project:masterfrom
Phmonski:feat/hs3-export-interpolation-information

Conversation

@Phmonski

@Phmonski Phmonski commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Summary

This PR adds bidirectional RooFit support for the structured interpolation representation introduced in hep-statistics-serialization-standard/hep-statistics-serialization-standard#103.

RooFit now exports interpolation behavior using the descriptive HS3 {type, in, out} structure instead of RooFit-specific integer codes. Import remains backward compatible with legacy integer interpolation fields and files relying on the previous implicit defaults.

Main changes

  • Add an internal structured interpolation descriptor with validated type, in, and nullable out fields.
  • Add class-specific mappings for FlexibleInterpVar and PiecewiseInterpolation.
  • Export a channel-level default_interpolation, selected from the most common interpolation behavior across normsys and histosys modifiers.
  • Omit modifier-level interpolation when it matches the channel default.
  • Emit an explicit structured interpolation object for modifiers that differ from the default.
  • Prefer {type: mult, in: poly6, out: exp} when frequencies are tied, followed by deterministic descriptor ordering.
  • Omit default_interpolation when a channel has no eligible modifiers.
  • Compare duplicate modifiers by interpolation behavior rather than their raw RooFit codes, allowing semantic aliases such as FlexibleInterpVar codes 4 and 5.
  • Add a PiecewiseInterpolation constructor accepting one interpolation code per parameter. This allows mixed histosys interpolation codes and repeated parameters to round-trip faithfully.

No interpolation mathematics or persistent class data layouts are changed.

Supported mappings

HS3 interpolation PiecewiseInterpolation FlexibleInterpVar
{add, poly1, null} 0 0
{mult, exp, null} 1 1
{add, poly2, poly1} 2; legacy 3 alias 2; legacy 3 alias
{add, poly6, poly1} 4 unsupported
{mult, poly6, exp} 5 4 or 5; imported canonically as 4
{mult, poly6, poly1} 6 unsupported

The class-dependent interpretation is important because RooFit code 4 has different semantics in PiecewiseInterpolation and FlexibleInterpVar.

Import compatibility

Interpolation is resolved in the following order:

  1. Modifier-level structured object or legacy integer.
  2. Channel-level default_interpolation.
  3. The legacy class-specific implicit code 4.

Structured objects must contain all three fields, including an explicit "out": null where appropriate. Malformed, unknown, and class-unsupported interpolation descriptions produce contextual errors identifying the channel, sample, and modifier.

RooFit exports only the new structured representation.

Tests

The tests cover:

  • Every supported mapping and legacy alias.
  • Canonicalization of legacy aliases.
  • Modal channel defaults and deterministic ties.
  • Explicit modifier overrides.
  • Channels without eligible modifiers.
  • Mixed histosys codes in one PiecewiseInterpolation.
  • Numerical round trips at interior, anchor, and extrapolation points.
  • Legacy integer fields and implicit defaults for both modifier classes.
  • Malformed, unknown, and class-unsupported interpolation objects.
  • Duplicate-modifier canonicalization using semantically equivalent codes.
  • Repeated parameters with per-entry PiecewiseInterpolation codes.
    @cburgard @stalbrec

@github-actions

github-actions Bot commented Jul 17, 2026 •
edited
Loading

Copy link
Copy Markdown

Test Results

    23 files      23 suites   3d 16h 56m 30s ⏱️
 3 877 tests  3 877 ✅ 0 💤 0 ❌
78 960 runs  78 960 ✅ 0 💤 0 ❌

Results for commit b080fa3.

♻️ This comment has been updated with latest results.

@guitargeek guitargeek changed the title [HS3] Add structured interpolation support for HistFactory modifiers [RF][HS3] Add structured interpolation support for HistFactory modifiers Jul 20, 2026
@Phmonski
Phmonski force-pushed the feat/hs3-export-interpolation-information branch from ad9e6ac to 4535201 Compare July 23, 2026 13:34
@Phmonski Phmonski mentioned this pull request Jul 24, 2026
Open
28 tasks
@Phmonski

Phmonski commented Jul 24, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

There is still some interpolation codes that need to be adressed:
class FlexibleInterpVarStreamer still writes:

elem["interpolationCodes"].fill_seq(fip->interpolationCodes());

root/roofit/hs3/src/JSONFactories_HistFactory.cxx

Line 646 in 43c20a5

class FlexibleInterpVarStreamer : public RooFit::JSONIO::Exporter {

@cburgard i assume this should also follow the interpolation conventions of the histfactory modifiers?

@cburgard

cburgard commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

There is still some interpolation codes that need to be adressed: class FlexibleInterpVarStreamer still writes:

elem["interpolationCodes"].fill_seq(fip->interpolationCodes());

root/roofit/hs3/src/JSONFactories_HistFactory.cxx

Line 646 in 43c20a5

class FlexibleInterpVarStreamer : public RooFit::JSONIO::Exporter {

@cburgard i assume this should also follow the interpolation conventions of the histfactory modifiers?

yes, that would be ideal

@Phmonski
Phmonski force-pushed the feat/hs3-export-interpolation-information branch from 4535201 to b080fa3 Compare July 27, 2026 09:05
@cburgard

cburgard commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

@Phmonski is this ready to be reviewed?

@Phmonski

Phmonski commented Jul 29, 2026

Copy link
Copy Markdown
Contributor Author

Yes, It should be ready.

@cburgard

cburgard commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

thanks!

@guitargeek would you mind reviewing this PR?

Phmonski and others added 6 commits July 29, 2026 14:22
@Phmonski @guitargeek
[RF][HS3] Add interpolation export for HistFactory modifiers
297baa6
@Phmonski @guitargeek
[RF][HS3] Add interp. IO for FlexibleInterpVar + PiecewiseInterpolation
74278ec
@Phmonski @guitargeek
[RF][HS3] Apply changes suggested by clang-format
54d8385
@guitargeek
[RF][HS3] Consolidate HistFactory interpolation code mapping into a t…
4fd7ed8 
…able

The mapping between the structured HS3 interpolation descriptors and the
RooFit integer codes was spread across four functions (two per direction,
one pair per interpolation class) plus a pair of dispatchers. They all
encoded the same table and had to be kept mutually consistent by hand.

Replace them with a single static table that mirrors the documented
mapping, plus two generic lookups. The class-specific quirks (legacy code
3 aliasing code 2, and FlexibleInterpVar treating code 4 and 5
identically) are now captured by two explicit normalization lines rather
than being scattered across switch cases.

No behavioural change; all HS3 interpolation tests still pass.
@guitargeek
[RF][HF] Remove dead commented-out code from PiecewiseInterpolation
a12f242 
The analytical-integral routines carried several large commented-out
blocks (old dirty-inhibit experiments, an abandoned numeric integral
attempt, and two obsolete per-code interpolation loops), and the file
ended with a fully commented-out printMetaArgs implementation. None of it
is compiled; remove it. The git history preserves the old code for anyone
who wants to revisit those ideas.

No behavioural change.
@guitargeek
[RF][HF] Share interpolation-code validation between the two classes
7d3dd58 
FlexibleInterpVar and PiecewiseInterpolation had an almost identical
setInterpCodeForParam(): the same range check, the same code-3 -> code-2
aliasing (with the same explanatory comment), and the same error
messages, differing only in the highest supported code and the class name
in the log output. Keeping the code-3 rule correct meant editing it in two
places in lockstep.

Extract the shared logic into a small internal helper,
Detail::setInterpolationCode(), used by both classes. Each override now
just forwards its parameter list and maximum code and flags itself
value-dirty when a code was stored.

No behavioural change; the error messages are preserved verbatim.
@guitargeek
guitargeek force-pushed the feat/hs3-export-interpolation-information branch from b080fa3 to 7d3dd58 Compare July 29, 2026 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@guitargeek guitargeek Awaiting requested review from guitargeek guitargeek is a code owner
@hageboeck hageboeck Awaiting requested review from hageboeck hageboeck is a code owner

At least 1 approving review is required to merge this pull request.

Assignees

@guitargeek guitargeek

Labels

in:RooFit

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Phmonski @cburgard @guitargeek