[HS3] Bugfixes to HS3 histfactoy_dist handling

[Phmonski]

This Pull request:

Patch to HS3 histfacory import and export:

• added a default error for normsys modifier
• changes the calculation of data.errors.
• the generic RooGaussian streamer function exported named RooConstVars as RooRealVars with isConstant(True). Changed the behavior to export RooConstVars as literals.

Changes or fixes:

• Histfactory normsys modifier get a default value of 0.0
• Previously, Histfactory used "constrain_p->getX()" to calculate the `erel for export. This resulted in issues when global observables changed in between loading and exporting the Workspace.
• Added a RooGaussianStreamer class that discriminates between RooRealVars and RooConstVars at export.

Checklist:

• tested changes locally

@cburgard @will-cern

[github-actions]

Test Results

    20 files      20 suites   3d 2h 59m 38s ⏱️
 3 848 tests  3 848 ✅ 0 💤 0 ❌
70 319 runs  70 319 ✅ 0 💤 0 ❌

Results for commit 7d204dc.

♻️ This comment has been updated with latest results.

[guitargeek]

Thank you for the PR! The reason why I didn't merge this yet is because I have a doubt with the treatment of the RooConstVar. It's a delicate topic, because it's not clear what the semantic difference of a RooConstVar and a constant RooRealVar should be.

So far, the policy has been:

• Export RooRealVars by name
• Export RooConstVar by value if the name matches the value, otherwise export by name (here is the logic to distinguish this)

What you are suggesting now is to change the policy only for the RooGaussian to always export RooConstVar by value, which seems a bit ad-hoc to me. What is the motivation to change the policy just for this class? Maybe we should change the policy in general, to keep things consistent?

@Phmonski, @cburgard

[Phmonski]

I think the Issue in that resulted in this particular change was that a named RooConstVar was exported and re-imported as a const RooRealVar, because of the default streamer behavior. Only RooConstVars where the name and value are numbers are exported as literals.

import ROOT

# Build two Gaussian PDFs, one with a constant sigma and one with a literal sigma value
x = ROOT.RooRealVar("x", "x", 0, 10)
sigma = ROOT.RooConstVar("sigma", "sigma", 1)
mean = ROOT.RooRealVar("mean", "mean", 5, 0, 10)
mean.setConstant(True)
gauss = ROOT.RooGaussian("gauss", "gauss", x, mean, sigma)
gauss_literal = ROOT.RooGaussian("gauss_literal", "gauss_literal", x, mean, 1.0)

# Create a workspace and import the PDFs
ws = ROOT.RooWorkspace("ws")
ws.Import(gauss)
ws.Import(gauss_literal)

# Export the workspace to JSON and import it back into a new workspace
tool = ROOT.RooJSONFactoryWSTool(ws)
tool.exportJSON("test.json")

ws2 = ROOT.RooWorkspace("ws2")
tool2 = ROOT.RooJSONFactoryWSTool(ws2)
tool2.importJSON("test.json")
ws2.Print()
ws2.var("sigma").Print()

[#0] WARNING:InputArguments -- The parameter 'sigma' with range [-inf, inf] of the RooGaussian 'gauss' exceeds the safe range of (0, inf). Advise to limit its range.

RooWorkspace(ws2) ws2 contents

variables
---------
(mean,sigma,x)

p.d.f.s
-------
RooGaussian::gauss[ x=x mean=mean sigma=sigma ] = 1
RooGaussian::gauss_literal[ x=x mean=mean sigma=1 ] = 1

parameter snapshots
-------------------
default_values = (mean=5[C],x=5,sigma=1[C])

RooRealVar::sigma = 1 C  L(-INF - +INF)

Some earlier Workspaces named their "_sigma" RooConstVar parameters in constraint terms which lead to reimporting them as RooRealVars. This raised some warnings with the xRooFit importer.

@cburgard

[guitargeek]

Ah, I see! Well, also from the perspective of round-tripping, I think what is now in the PR is not what we want, because the original name of the RooConstVar gets lost on the JSON export.

What about storing in the "misc" which variable names are RooConstVar? That would help to keep the round-tripping for RooFit consistent.

[Phmonski]

I can add a new attribute called misc.ROOT_internal.attributes.<name>.is_const_var = 1, for named RooConstVars.
Then on import, if a variable has that marker, create a RooConstVar instead of a constant RooRealVar.

What do you think @cburgard ?

[Phmonski]

I have Updated the the branch to store the Information on RooConstVars in the misc part of the HS3 files but as you can see in the diff, there are some comments that suggest HS3 doesn't want to implement RooConstVars.
So I would like for @cburgard to comment on this change

[cburgard]

Since I was tagged here I'll share my take on things. That being said, I don't feel I'm neccessarily the most competent person to comment here.

RooConstVar is a strange beast from a statistics perspective, as there is not really a definition of what it should be. It is a constant value, that should basically never change - but so is a RooAbsReal with the "const" property set. The only real difference, then, is the amount of work someone needs to do to change the value, which doesn't really translate well to other frameworks.

Looking at HS3, the best mapping of existing constructs I can find is that a RooAbsReal (const or no const) is a variable that has a domain and a value in the default_values parameter point, whereas a RooConstVar is just a numeric literal. That mapping works well whenever there is a "true" RooConstVar, that is, one that has no information about it other than its value. The main reason why they exist, from my understanding, is that RooConstVar does not show up in the dependency graph, which helps keep some computations more lightweight - that's a very ROOT-like feature that does not translate well to other frameworks, though.
Now, named RooConstVars are strange in that they have a name that allows to refer to them, but there is nothing this name "does" other than allow access to a fixed number. Fundamentally, I can see the following paths forward:

1. All RooConstVars become numeric literals, the names are dropped. This is the cleanest mapping, but it does not preserve naming on roundtrips. That's a choice we could make, but not ideal.
2. All RooConstVars become variables with a const attribute. Conventionally, this will make them show up as RooRealVars on reimport. Also not ideal, as roundtripping breaks just as well.
3. There is some logic under which RooConstVars will be exported as literals, and some logic under which they will be exported as proper variables with the const attribute - hopefully in such a way that it is reversible. This one has the chance of preserving roundtripping at the cost of more complexity.

Both 1 and 2 could potentially be augmented by putting information into misc, but I would want to use that capability as sparingly as possible. Of these two, (1) seems more appealing to me if we do not find a solution for (3), as with a loss of misc, getting the name of an object wrong that supports no operation other then getVal seems less catastrophic than changing the dependency graph by introducing new fully-featured leafs to the computational graph that did not exist before.

Let's talk 3, then. For now, we have used the logic of "named vs. unnamed" to decide which one we want to do: literal or variable. That was an easy choice, because that is exactly the condition under which we are able to supply a name on the hs3 side. This had the downside of having no way to distinguish between proper variables and former ConstVars on the side of the import.
If we try to deal with this functionally though, I would suggest this: The thing that makes a ConstVar different from a RealVar is the fact that it never changes the value. This means that it has no range.
To me, the most natural way to detect things that were ConstVars before (or, for that matter, should behave like a ConstVar when the HS3 file was produced by something else then ROOT) would be to detect if there is any indication that the variable should ever change value. Hence, I would suggest the following:

• Export unnamed RooConstVars as literals.
• Export named RooConstVars as variables, but refrain from defining a domain.
• On import, check all domains in the file: if a variable does not have any domain set, flag it as a RooConstVar.
  That, to me, would leverage the power of the RooConstVar (efficient computations through leaner dependency graphs) also in cases where the origin of the HS3 file is not within ROOT, and without trying to leverage some metainformation from misc.

I hope that makes sense.

[guitargeek]

Thanks @cburgard for chiming in! This is a good suggestion to use the domains for that. But I'm also worried that then we're running into ambiguities with RooRealVars that have unlimited range, in which case we might want to skip the "domains" entry for brevity.

By the way, the benefit of RooConstVar is also that they are mostly hidden from the user as well: they don't show up in RooWorkspace::Print() and are also not listed as non-floating parameters in fit results.

So taking a step back, I'm wondering where it's even useful to have named RooConstVars as concepts in RooFit in general. I guess in some cases one could add information about the model semantics by "attaching" a name to a constant, but given that this name will usually be hidden anyway this is only useful when one directly verbose-prints the dependent pdf or function.

So this is the other way out I see:

• Always write RooConstVar out by value and drop the name
• Audit all RooFit and HistFactory code and tutorials to not use names RooConstVars anymore to avoid round-tripping problems
• Maybe even add a warning if one creates a RooConstVar with a name that doesn't match the value and refer to using RooFit::RooConst(<value>), which is the helper to create RooConstVars with a name that matches the value

What do you think about that? Also tagging @will-cern. because he might use the meta info from RooConstVar names in xRooFit.

[will-cern]

Just to throw a term in here that we came up before for these ConstVars ... at some point we started calling them "prespecified parameters". They are things that have a value but can never "float".

I am happy with the idea that these types of parameters are "variables" in HS3 that do not have a domain (no range), assuming that an infinite domain (+/- inf) is distinct from having no domain. RooConstVars don't have "binnings" (the roofit equivalent of what I am reading here to be a 'domain').

Does that all tally with what others are thinking?

[cburgard]

Yes, I agree with @will-cern here.
As for the domains: I think the sane option would be to force every RooRealVar to have a domain anyway, even if it's unbounded. I think some people have complained about the absence of domains for unbounded variables anyway. See hep-statistics-serialization-standard/hep-statistics-serialization-standard#95, point A1.
What I take from this is that an unbounded RooRealVar should have an "empty" domain entry (no min/max, just the name), whereas the RooConstVar should have no domain entry at all.

[guitargeek]

Okay thanks, for the discussion! If a RooRealVar with unlimited range is distinct from a variable with no domain in HS3 then I have no further doubt with the solution that Carsten suggested.

@Phmonski can you implement the suggested tagging of named RooConstVar by absence of a domain in this PR?

[Phmonski]

Ill gladly implement this suggestion.

Just for my understanding
On Export:

1. RooConstVars where name and value match, stay as literals in HS3
2. Named RooConstVars are saved as Variables without domain entry
3. Force all RooRealVars to get a domain entry, even constant and unbounded ones,

On Import:

1. Parameters without a domain entry are created as RooConstVar

@will-cern @cburgard @guitargeek

[cburgard]

Ill gladly implement this suggestion.

Just for my understanding On Export:

1. RooConstVars where name and value match, stay as literals in HS3
2. Named RooConstVars are saved as Variables without domain entry
3. Force all RooRealVars to get a domain entry, even constant and unbounded ones,

On Import:

1. Parameters without a domain entry are created as RooConstVar

@will-cern @cburgard @guitargeek

Yes exactly.
Just one minor thing: the aggressive conversion of everything that does not have a domain to rooconstvar should be a global flag. That way, we can easily "upconvert" models published with the old convention.

[Phmonski]

@cburgard does this match your expectation?
I set the default for the flag as true, so all no-domain parameters are converted to RooConstVars