search

root-project / root

The official repository for ROOT: analyzing, storing and visualizing big data, scientifically
https://root.cern
Other
2.63k stars 1.26k forks source link

[RF] Inconsistent behavior when editing constraint terms in HistFactory models #9070

Closed alexander-held closed 2 years ago

alexander-held commented 2 years ago

Describe the bug

The HistFactory xml specification allows for editing constraint terms of nuisance parameters in the measurement with constructions like:

<ConstraintTerm Type="Uniform" RelativeUncertainty="1">NP_norm</ConstraintTerm>

Constraint terms can be removed from OverallSys modifiers, but the same method does not seem to work for HistoSys modifiers (and the setting seems to be silently ignored).

Constraint term "removal" can be achieved by changing the constraint term to be uniform, but RooStats::HistFactory::Measurement::AddNoSyst also provides an alternative method. The xml created from that seems to be incompatible with hist2workspace, as the xml uses NoSyst and hist2workspace seems to (at least parse) NoConstraint instead, but crashes for NoSyst.

Expected behavior

I expect constraint term editing to work for both OverallSys and HistoSys modifiers. Both approaches of changing the constraint term to be uniform and removing it completely should work with hist2workspace.

To Reproduce

Download the required input files from https://cernbox.cern.ch/index.php/s/AO4ruN3G7LNtcRo. They contain a minimal HistFactory workspace specification (xml + ROOT file with histograms) and a simple script to fit the workspace.

Baseline for comparisons

To establish the baseline behavior, run hist2workspace on the provided HistFactory workspace (xml + ROOT) and perform a fit:

cd input
hist2workspace minimal_example.xml
cd ..
root -l -q fit.cxx

The relevant best-fit parameter values are:

  NO.   NAME            VALUE
   1  alpha_NP_norm  -4.66233e-01
   2  alpha_NP_shape -1.70517e-01
   3  mu              1.04248e+00

OverallSys with uniform constraint [working as expected]

To see the behavior of the OverallSys modifier when removing the associated constraint term, edit the xml and add the ConstraintTerm configuration. The measurement should then look like this:

  <Measurement Name="minimal_example" Lumi="1" LumiRelErr="0.1" ExportOnly="True">
    <POI>mu</POI>
    <ParamSetting Const="True">Lumi</ParamSetting>
    <ConstraintTerm Type="Uniform" RelativeUncertainty="1">NP_norm</ConstraintTerm>
  </Measurement>

Repeat the workspace conversion and fitting:

cd input
hist2workspace minimal_example.xml
cd ..
root -l -q fit.cxx

and the result is now

  NO.   NAME            VALUE
   1  alpha_NP_norm  -6.61703e-01
   2  alpha_NP_shape -2.12313e-01
   3  mu              1.10747e+00

where the pull for alpha_NP_norm has increased, which makes sense given the removal of its constraint term.

HistoSys with uniform constraint [not working as expected]

To see the behavior of the HistoSys modifier when removing the associated constraint term, edit the xml and add the ConstraintTerm configuration for NP_shape. The measurement should then look like this:

  <Measurement Name="minimal_example" Lumi="1" LumiRelErr="0.1" ExportOnly="True">
    <POI>mu</POI>
    <ParamSetting Const="True">Lumi</ParamSetting>
    <ConstraintTerm Type="Uniform" RelativeUncertainty="1">NP_shape</ConstraintTerm>
  </Measurement>

Repeat the workspace conversion and fitting:

cd input
hist2workspace minimal_example.xml
cd ..
root -l -q fit.cxx

and the result is again

  NO.   NAME            VALUE
   1  alpha_NP_norm  -4.66233e-01
   2  alpha_NP_shape -1.70517e-01
   3  mu              1.04248e+00

which is exactly the same as the initial result when

<ConstraintTerm Type="Uniform" RelativeUncertainty="1">NP_shape</ConstraintTerm>

was not used. The setting seems to have been ignored.

Using NoSyst [crashing, unexpected behavior in workaround]

In addition to Uniform, NoSyst is also available: https://github.com/root-project/root/blob/c2c918c2662a02f3668b5044007e12d220133e71/roofit/histfactory/src/Measurement.cxx#L401-L406

When using the following measurement (Uniform replaced by NoSyst):

  <Measurement Name="minimal_example" Lumi="1" LumiRelErr="0.1" ExportOnly="True">
    <POI>mu</POI>
    <ParamSetting Const="True">Lumi</ParamSetting>
    <ConstraintTerm Type="NoSyst" RelativeUncertainty="1">NP_norm</ConstraintTerm>
  </Measurement>

hist2workspace crashes:

[#2] ERROR:HistFactory -- Error: Encountered unknown type for ConstraintTerm: NoSyst
HistFactory - Exception
hist2workspace - Caught Exception: HistFactory - Exception

In ConfigParser.cxx, NoConstraint appears: https://github.com/root-project/root/blob/c2c918c2662a02f3668b5044007e12d220133e71/roofit/histfactory/src/ConfigParser.cxx#L509-L514

When using that via

<ConstraintTerm Type="NoConstraint" RelativeUncertainty="1">NP_norm</ConstraintTerm>

hist2workspace runs but the parameter for NP_norm disappears from the fit results.

Setup

ROOT 6.24/06 in a container built with Debian 10

Additional context

none

alprades commented 2 years ago

Hi all,

My name is Alberto Prades Ibañez and I am an ATLAS PhD student who is doing a top quark mass measurement. For my analysis I have implemented a Likelihood Unfolding approach where I have a set of nuisance parameters for the uncertainties (with their gaussian priors) and a Nuisance Parameter for the Top MC mass (with a uniform/flat prior).

While talking with the experts (Tomas Dado and @alexander-held ) I was told that the uniform prior had a bug. The bug is explained in this issue but it seems it was never been solved. I would to encourage the HistFactory/Root team to solve it because it is a really important feature for our analysis (and I am sure that also for many more analysis that will come in the future).

Cheers,

Alberto Prades

MihaMuskinja commented 2 years ago

Hi all,

I am also interested in this. Is there perhaps a workaround that would enable us to have an unconstrained shape-only nuisance parameter?

Best, Miha

guitargeek commented 2 years ago

Hi! Thanks for reminding me of this issue. I missed it last year, as it didn't get RooFit/RooStats label, sorry!

I have opened a PR to fix the inconsistency: https://github.com/root-project/root/pull/10525

And you are right, the label for constant nuisance parameters are not consistent on the XML and C++ side. As you noticed, in XML it's NoConstraint and in C++ Measurement class the label NoSyst is used, however, I would not change this in HistFactory because changing this longstanding naming might surprise users.

Let me know if this is good for you and feel free to open new issues in case you have any other problems!

github-actions[bot] commented 2 years ago

Hi @guitargeek, @lmoneta,

It appears this issue is closed, but wasn't yet added to a project. Please add upcoming versions that will include the fix, or 'not applicable' otherwise.

Sincerely, :robot:

alexander-held commented 2 years ago

Thanks a lot @guitargeek!

And you are right, the label for constant nuisance parameters are not consistent on the XML and C++ side. As you noticed, in XML it's NoConstraint and in C++ Measurement class the label NoSyst is used, however, I would not change this in HistFactory because changing this longstanding naming might surprise users.

Sticking with the existing names sounds good to me, I wouldn't want to break existing code.