Skip to content

feat: Improvements based on arXiv:2504.07686 files#187

Open
jackaraz wants to merge 5 commits into
scipp-atlas:mainfrom
jackaraz:updates
Open

feat: Improvements based on arXiv:2504.07686 files#187
jackaraz wants to merge 5 commits into
scipp-atlas:mainfrom
jackaraz:updates

Conversation

@jackaraz

Copy link
Copy Markdown

These changes are based on the HS3 file provided with arXiv:2504.07686. can be downloaded via

curl -OJLH "Accept: application/json" https://doi.org/10.17182/hepdata.157266.v1/r5

@kratsg, please let me know if these break generality. I'm trying to work on a case-by-case basis.

Error summary

Running pyhs3.Workspace(**ws_dict) on likelihood-dphijj-cp.json raises a billion pydantic ValidationError

# Category Occurrences pyhs3 class
1 staterror modifier missing required data field 810 StatErrorModifier
2 SampleData.errors field missing (optional in HS3) 2396 SampleData
3 Unknown modifier type "custom" 2036 ModifierType union

Issue 1: staterror modifier: data field incorrectly required

{
  "constraint_type": "Poisson",
  "name": "staterror",
  "parameters": [
    "auto_gamma_stat_CR_0j_DF_DNN_ptH_0_10_WW_bin_0_HWW_ggFVBF_01jDF_ggFVBF_01jDF"
  ],
  "type": "staterror"
}

The modifier specifies the per-bin gamma parameter names in parameters and the constraint type via constraint_type, but carries no data object. The per-bin statistical uncertainties are instead stored on the sample's own data:

"data": {
  "contents": [50.3, ...],
  "errors":   [7.0, ...]
}

StatErrorModifier (in modifiers.py) is defined as:

class StatErrorModifier(HasConstraint, ParametersModifier):
    type: Literal["staterror"] = "staterror"
    parameters: list[str]
    constraint: Literal["Gauss"] = "Gauss"
    data: StatErrorData          # ← no default

StatErrorData requires:

class StatErrorData(ModifierData):
    uncertainties: list[float]   # ← must be supplied explicitly

Because there is no data key in the file's staterror object, Pydantic raises Field required a billion times.

HS3 standard

The HS3 v0.2.9 specification (§HistFactory) describes staterror as:

"a special subtype of shapefactor, where the mean of the constraint is given
as the sum of the predictions of all the samples carrying a staterror modifier
in this bin."

Crucially, the spec's normative example for staterror contains only constraint and parameters keys no data field. The per-bin uncertainties are not embedded in the modifier; they are taken implicitly from the parent sample's errors array (the Barlow-Beeston method). Requiring an explicit data.uncertainties block in the modifier is therefore an internal pyHS3 design choice that conflicts with the standard.

Two further discrepancies exist between the ATLAS file and phys3:

  • constraint_type vs constraint: The ATLAS file uses constraint_type: "Poisson", which matches the HS3 spec field name. pyhs3's StatErrorModifier uses the internal attributeconstraintand hardcodes it to"Gauss"`, so even if the field were parsed, it would be silently ignored.
  • Poisson staterror: The spec explicitly allows "Poisson" as a valid constraint type for staterror, but pyhs3 only accepts Literal["Gauss"].

Issue 2: SampleData.errors incorrectly required

Many samples omit the errors key entirely:

"data": {
  "contents": [0.553, 1.204, ...]
}
class SampleData(BaseModel):
    contents: list[float]
    errors:   list[float]    # ← REQUIRED, no default

Because Pydantic enforces this strictly, every sample without errors triggers a Field required validation error.

The HS3 v0.2.9 specification describes sample data as:

"struct containing the components 'contents' and 'errors', depicting the
data contents and their errors."

While both fields are named, the spec does not mark errors as normatively required; it describes what the field represents rather than mandating its presence. Real-world ATLAS workspaces routinely omit errors for samples where MC statistical uncertainties are negligible or handled via a global staterror. When absent, the interpretation is that per-bin errors are zero (infinite MC statistics).

Issue 3: custom modifier type not implemented

{
  "name": "combPdf_16",
  "type": "custom"
}

The modifier's name is the identifier of an entry in the top-level functions section. For this file, every such reference resolves to a generic_function:

{
  "expression": "mu_VBF_qq2Hqq_1J_param",
  "name": "combPdf_16",
  "type": "generic_function"
}

In other words, custom is an indirection mechanism: instead of embedding normalisation logic inside the modifier itself, the modifier delegates to a named function that is already registered in the workspace. This allows the reuse of complex normalisation expressions (e.g. multi-POI signal strengths, combined normalisation products) across many samples without duplication.

Changes made

Fix 1: distributions/histfactory/samples.py

  • SampleData.errors optional
    errors: list[float] → errors: list[float] | None = None, with a model_validator that fills zeros when absent. The test test_sampledata_requires_errors was updated to assert the new correct behaviour (defaults to zeros).

Fix 2: distributions/histfactory/modifiers.py

StatErrorModifier three sub-fixes

  • data optional: data: StatErrorDatadata: StatErrorData | None = None. When absent, make_constraint derives uncertainties from the parent sample_data.errors array (the Barlow-Beeston path described in the HS3 spec).
  • constraint_type alias: a model_validator(mode='before') maps the ATLAS field name constraint_type → internal constraint before pydantic parses the model.
  • Poisson support: constraint: Literal["Gauss"]Literal["Gauss", "Poisson"], with a separate code path in make_constraint that builds Poisson Barlow-Beeston constraints (gamma × tau ~ Poisson(tau)) instead of the Gaussian path.

Fix 3: distributions/histfactory/modifiers.py

  • CustomModifier added a new class whose name is the workspace function identifier. dependencies returns {self.name} so the compiler evaluates the function first; apply multiplies rates by context[self.name]. Registered at the front of the ModifierType discriminated union.

@jackaraz
jackaraz requested a review from kratsg as a code owner March 10, 2026 21:08
@kratsg kratsg changed the title Improvements based on arXiv:2504.07686 files feat: Improvements based on arXiv:2504.07686 files Mar 11, 2026
@kratsg

kratsg commented Mar 11, 2026

Copy link
Copy Markdown
Contributor

One quick note -- pyhs3.Workspace.load("likelihood-dphijj-cp.json") will give you more human-friendlier errors.

Fix 1: distributions/histfactory/samples.py

For this one, see #109 (comment) . An array of zeros is the correct choice. One needs to preserve round-tripping by not having the error written out for the sample if it is an array of all zeros as well.

Fix 2: distributions/histfactory/modifiers.py

Related to hep-statistics-serialization-standard/hep-statistics-serialization-standard#68 (comment) .

For constraint vs constraint_type -- it is supposed to be constraint. The JSON file you have is incorrect and that needs to be flagged and fixed with the ROOT team.

For additional support of other constraints, it would be great to get this (related: #138) . I had a plan which was to just generate the mathematical expressions in ROOT and print them out and port them over, since the existing documentation here is not abundantly clear on how to ensure they are implemented correctly.

Fix 3: distributions/histfactory/modifiers.py

This is not in the HS3 spec -- https://hep-statistics-serialization-standard.github.io/parts/histfactory/.


If you're using AI, make sure you acknowledge which one. And make sure all tests pass and coverage is 100% and no type issues.

@kratsg kratsg linked an issue Apr 4, 2026 that may be closed by this pull request
@kratsg

kratsg commented Apr 4, 2026

Copy link
Copy Markdown
Contributor

Issue 1 can be fixed.
Issue 2 is fixed with #197.
Issue 3 cannot be fixed. I don't know how you're meant to handle or support custom. You would need to insert your custom distribution or function into the registry.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Status of histfactory_dist distributions

2 participants