Skip to content

[RF][HS3] Align RooFit HS3 JSON I/O with the current HS3 naming and HistFactory conventions.#22735

Open
Phmonski wants to merge 5 commits into
root-project:masterfrom
Phmonski:fix/hs3-spec-alignment
Open

[RF][HS3] Align RooFit HS3 JSON I/O with the current HS3 naming and HistFactory conventions.#22735
Phmonski wants to merge 5 commits into
root-project:masterfrom
Phmonski:fix/hs3-spec-alignment

Conversation

@Phmonski

@Phmonski Phmonski commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Summary

Align RooFit HS3 JSON I/O with the current HS3 naming and HistFactory conventions.

  • Replace RooBinWidthFunction HS3 type binwidth with:
    • binvolume for divideByBinWidth == false
    • inverse_binvolume for divideByBinWidth == true
  • Stop exporting divideByBinWidth and remove legacy binwidth import support.
  • Extend generic expression cleanup for floor, ceil, abs, tan, asin, acos, atan, PI, and EULER.
  • Treat PI and EULER as reserved expression constants on import.
  • Migrate HistFactory modifier export from constraint_name to constraint, and omit constraint_type.
  • Instead of exporting constraint_type, shapesys modifiers now export the actual per-bin constraint pdf names via a new constraints list. This list is parallel to the parameters list, so each gamma parameter has a corresponding constraint entry. Entries can also be null for parameters without a constraint.
  • Keep HistFactory import compatibility for constraint, constraint_name, and legacy constraint_type.
  • Export const: true for parameters whose min >= max.
  • Export both RooHistFunc and ParamHistFunc as step.
  • Add a step importer dispatcher that selects RooHistFunc for data and ParamHistFunc for parameters.
  • Keep legacy histogram import support for old RooHistFunc HS3 files.
  • Rename the following exporter keys:
    • generic_function -> generic
    • gauss_model_function -> gauss_resolution_model
    • truth_model_function -> delta_resolution_model
    • mixture_model -> mixture_resolution_model
    • fft_conv_pdf -> fft_convolution_dist

Tests

Added focused coverage in testRooFitHS3.cxx for:

  • binvolume / inverse_binvolume export and import
  • fixed-range parameter export as const: true
  • step export/import dispatch for RooHistFunc and ParamHistFunc
  • HistFactory constraint export with legacy constraint_name import
  • generic expression cleanup for new function names and constants

[x] Tested locally

@github-actions

github-actions Bot commented Jul 1, 2026

Copy link
Copy Markdown

Test Results

    2 files      2 suites   2h 16m 58s ⏱️
2 603 tests 2 603 ✅ 0 💤 0 ❌
4 887 runs  4 887 ✅ 0 💤 0 ❌

Results for commit a53e7ad.

♻️ This comment has been updated with latest results.

@Phmonski

Phmonski commented Jul 2, 2026

Copy link
Copy Markdown
Contributor Author

This PR contains a bug that is investigated.

@Phmonski Phmonski force-pushed the fix/hs3-spec-alignment branch 2 times, most recently from 636e449 to 98c0bd8 Compare July 6, 2026 09:16
@Phmonski

Phmonski commented Jul 6, 2026

Copy link
Copy Markdown
Contributor Author

I traced the gtest-roofit-histfactory-testHistFactory HS3 closure failure to the removal of constraint_type from the HistFactory export.

The problem is not that we need to keep exporting constraint_type; we do not want that because it is no longer part of the HS3 spec. The issue is that constraint_type was previously the only information that preserved the constraint family for some ShapeSys modifiers.

In the failing test, one ShapeSys uses Gaussian constraints and another uses Poisson constraints. During export, ShapeSys currently serializes the relative uncertainties, but it does not serialize a concrete constraint pdf reference. Previously the exporter fell back to writing constraint_type. After removing that fallback, the imported model no longer knows whether the ShapeSys was Gaussian or Poisson. The importer then falls back to its default behavior, which effectively turns the Poisson ShapeSys into a Gaussian one. That breaks HS3 round-trip closure.

This seems mainly to affect ShapeSys, because it can represent multiple per-bin gamma constraints under one modifier and currently has no single exported constraint pdf name. Other modifiers such as normsys / histosys / normfactor usually still have a concrete named constraint pdf that can be exported via the spec-compliant constraint field. There may also be a similar latent issue for non-default staterror constraints if we need to preserve Gaussian vs Poisson there without constraint_type.

I can think of two possible fixes:

  1. Export the actual per-bin ShapeSys constraints as named distributions, possibly grouped with a product_dist, and reference that via constraint.
  2. Export the constraint_type as a new node e.g. constraint_family.

What would be the preferred way to preserve the model semantics?
@cburgard @stalbrec

@cburgard

cburgard commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

I traced the gtest-roofit-histfactory-testHistFactory HS3 closure failure to the removal of constraint_type from the HistFactory export.

The problem is not that we need to keep exporting constraint_type; we do not want that because it is no longer part of the HS3 spec. The issue is that constraint_type was previously the only information that preserved the constraint family for some ShapeSys modifiers.

In the failing test, one ShapeSys uses Gaussian constraints and another uses Poisson constraints. During export, ShapeSys currently serializes the relative uncertainties, but it does not serialize a concrete constraint pdf reference. Previously the exporter fell back to writing constraint_type. After removing that fallback, the imported model no longer knows whether the ShapeSys was Gaussian or Poisson. The importer then falls back to its default behavior, which effectively turns the Poisson ShapeSys into a Gaussian one. That breaks HS3 round-trip closure.

This seems mainly to affect ShapeSys, because it can represent multiple per-bin gamma constraints under one modifier and currently has no single exported constraint pdf name. Other modifiers such as normsys / histosys / normfactor usually still have a concrete named constraint pdf that can be exported via the spec-compliant constraint field. There may also be a similar latent issue for non-default staterror constraints if we need to preserve Gaussian vs Poisson there without constraint_type.

I can think of two possible fixes:

1. Export the actual per-bin `ShapeSys` constraints as named distributions, possibly grouped with a `product_dist`, and reference that via `constraint`.

2. Export the constraint_type as a new node e.g. constraint_family.

What would be the preferred way to preserve the model semantics? @cburgard @stalbrec

I'd say the correct answer is (1), and I would argue to add a key called "constriants" which should just be a list of the constraint names. That has the nice feature of introducing a parallelism with the parameters. If we can make it such that those lists are always synced by construction by potentially introducing "null" for the constraint of parameters that do not carry constraints, that would introduce a 1:1 mapping of parameters to constraints that might be useful for tools dealing with the JSONs.

@Phmonski Phmonski force-pushed the fix/hs3-spec-alignment branch from 98c0bd8 to 88cc345 Compare July 7, 2026 09:22
@Phmonski Phmonski requested a review from bellenot as a code owner July 7, 2026 10:59
@Phmonski Phmonski force-pushed the fix/hs3-spec-alignment branch from d6e1b63 to a53e7ad Compare July 7, 2026 11:44
@guitargeek guitargeek closed this Jul 7, 2026
@guitargeek guitargeek reopened this Jul 7, 2026
@guitargeek guitargeek changed the title [HS3] Align RooFit HS3 JSON I/O with the current HS3 naming and HistFactory conventions. [RF][HS3] Align RooFit HS3 JSON I/O with the current HS3 naming and HistFactory conventions. Jul 8, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants