Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RF] Values of RooProdPdfs in a RooAbsPdf should not depend on its factors normRange() #11486

Open
guitargeek opened this issue Oct 4, 2022 · 3 comments

Comments

@guitargeek
Copy link
Contributor

guitargeek commented Oct 4, 2022

Describe the bug

If a RooProdPdf is in a RooAddPdf, its values change if one changes the normalization range of its factors with RooAbsPdf::normRange().

Expected behavior

The values of a RooProdPdf should not depend on the normRange() of its factors, because the RooProdPdf is responsible for normalizing itself.

To Reproduce

Enable the new unit test in testRooProdPdf shipped with #11485.

Setup

ROOT master on Arch Linux.

Additional context

It is important now to fix the issues related to the RooAbsPdf::normRange() feature, because as of #11455 it is used in multi-range fits. Thus, we need to make sure it's less fragile.

After this issue is fixed, it should be verified with stressRooFit that things would still work if one were to set the normalization ranges of all PDFs in the computation graph in RooAbsOptTestStatistic::initSlave() not just the top-level PDF:

For debugging, it could be helpful to replace the RooProdPdfs with RooFixedProdPdf objects, which explicitly represent the conputation graph of a RooProdPdf for a given normalization set without internal caching:

         if(auto prodPdf = dynamic_cast<RooProdPdf *>(pdf)) {
         auto normalizedPdf = std::make_unique<RooFixedProdPdf>(*prodPdf, currNormSet);

         replaceArg(*normalizedPdf, *pdf);

         newNodes.emplace_back(std::move(normalizedPdf));

         continue;
         }
@guitargeek guitargeek self-assigned this Oct 4, 2022
@guitargeek guitargeek changed the title [RF] Values of RooProdPdfs in a RooAbsPdf should not depend on normRange() of its factors [RF] Values of RooProdPdfs in a RooAbsPdf should not depend its factors normRange() Oct 4, 2022
@guitargeek guitargeek changed the title [RF] Values of RooProdPdfs in a RooAbsPdf should not depend its factors normRange() [RF] Integrating a RooProdPdf without normalization set is broken Oct 4, 2022
@guitargeek guitargeek changed the title [RF] Integrating a RooProdPdf without normalization set is broken [RF] Integrating a RooProdPdf with and empty normalization set is broken Oct 4, 2022
@guitargeek guitargeek changed the title [RF] Integrating a RooProdPdf with and empty normalization set is broken [RF] Values of RooProdPdfs in a RooAbsPdf should not depend its factors normRange() Oct 4, 2022
@guitargeek guitargeek changed the title [RF] Values of RooProdPdfs in a RooAbsPdf should not depend its factors normRange() [RF] Values of RooProdPdfs in a RooAbsPdf should not depend on its factors normRange() Oct 6, 2022
@guitargeek
Copy link
Contributor Author

guitargeek added a commit to guitargeek/root that referenced this issue Jan 11, 2023
The `RooRealIntegral` class is smart enough to figure out which
variables the function the integrated function doesn't depend on and
trivially integrates them itself by multiplying with the variable
definition range.

However, if the integration range is a subrange of the variable range,
this was not considered. This resulted in wrong results. for integrals
like `pdf.createIntegral(x, "subrange")`, where the pdf doesn't depend
on x. These kind of integrals can occur in the projections that the
RooAddPdf does, so it's important that they work, and fixing this
partially addresses root-project#11486.

This change also fixes a so-far unknown bug in the `RooXYChi2Var`, which
also used these kind of integrals. Without this fix, the `Integrate()`
feature for `chi2FitTo()` was completely broken, which can be seen in
the output of the `rf609` tutorial with any previous ROOT version. The
tutorial looks okay by chance, because the function is dominted by the
quadratic term in `x` that is constant in the fit. But use can see in
the fit result that something is off. Unfortunately, this tutorial was
also turned into a unit test in stressRooFit, so the reference results
need to be updated.

To demonstrate that things work correctly now, a new unit test was
implemented that does the closure check of the `integrate()` feature of
the `RooXYChi2Var` with a linear fit function.
guitargeek added a commit to guitargeek/root that referenced this issue Jan 11, 2023
The `RooRealIntegral` class is smart enough to figure out which
variables the function the integrated function doesn't depend on and
trivially integrates them itself by multiplying with the variable
definition range.

However, if the integration range is a subrange of the variable range,
this was not considered. This resulted in wrong results. for integrals
like `pdf.createIntegral(x, "subrange")`, where the pdf doesn't depend
on x. These kind of integrals can occur in the projections that the
RooAddPdf does, so it's important that they work, and fixing this
partially addresses root-project#11486.

This change also fixes a so-far unknown bug in the `RooXYChi2Var`, which
also used these kind of integrals. Without this fix, the `Integrate()`
feature for `chi2FitTo()` was completely broken, which can be seen in
the output of the `rf609` tutorial with any previous ROOT version. The
tutorial looks okay by chance, because the function is dominted by the
quadratic term in `x` that is constant in the fit. But if one makes this
a floating parameter, the problem gets obvious.

Probably that was the reason why the main model parameter was set
constant to begin with, to sweep the bug under the rug. Now, the
tutorials are updated to have the quadratic coefficient floating too.

To demonstrate that things work correctly now, a new unit test was
implemented that does the closure check of the `integrate()` feature of
the `RooXYChi2Var` with a linear fit function.
guitargeek added a commit to guitargeek/root that referenced this issue Jan 11, 2023
The `RooRealIntegral` class is smart enough to figure out which
variables the function the integrated function doesn't depend on and
trivially integrates them itself by multiplying with the variable
definition range.

However, if the integration range is a subrange of the variable range,
this was not considered. This resulted in wrong results. for integrals
like `pdf.createIntegral(x, "subrange")`, where the pdf doesn't depend
on x. These kind of integrals can occur in the projections that the
RooAddPdf does, so it's important that they work, and fixing this
partially addresses root-project#11486.

This change also fixes a so-far unknown bug in the `RooXYChi2Var`, which
also used these kind of integrals. Without this fix, the `Integrate()`
feature for `chi2FitTo()` was completely broken, which can be seen in
the output of the `rf609` tutorial with any previous ROOT version. The
tutorial looks okay by chance, because the function is dominted by the
quadratic term in `x` that is constant in the fit. But if one makes this
a floating parameter, the problem gets obvious.

Probably that was the reason why the main model parameter was set
constant to begin with, to sweep the bug under the rug. Now, the
tutorials are updated to have the quadratic coefficient floating too.

To demonstrate that things work correctly now, a new unit test was
implemented that does the closure check of the `integrate()` feature of
the `RooXYChi2Var` with a linear fit function.
guitargeek added a commit to guitargeek/root that referenced this issue Jan 11, 2023
The `RooRealIntegral` class is smart enough to figure out which
variables the function the integrated function doesn't depend on and
trivially integrates them itself by multiplying with the variable
definition range.

However, if the integration range is a subrange of the variable range,
this was not considered. This resulted in wrong results. for integrals
like `pdf.createIntegral(x, "subrange")`, where the pdf doesn't depend
on x. These kind of integrals can occur in the projections that the
RooAddPdf does, so it's important that they work, and fixing this
partially addresses root-project#11486.

This change also fixes a so-far unknown bug in the `RooXYChi2Var`, which
also used these kind of integrals. Without this fix, the `Integrate()`
feature for `chi2FitTo()` was completely broken, which can be seen in
the output of the `rf609` tutorial with any previous ROOT version. The
tutorial looks okay by chance, because the function is dominted by the
quadratic term in `x` that is constant in the fit. But if one makes this
a floating parameter, the problem gets obvious.

Probably that was the reason why the main model parameter was set
constant to begin with, to sweep the bug under the rug. Now, the
tutorials are updated to have the quadratic coefficient floating too.
And also `stressRooFit`, since the reference file has to be updated
anyway.

To demonstrate that things work correctly now, a new unit test was
implemented that does the closure check of the `integrate()` feature of
the `RooXYChi2Var` with a linear fit function.
guitargeek added a commit that referenced this issue Jan 12, 2023
The `RooRealIntegral` class is smart enough to figure out which
variables the function the integrated function doesn't depend on and
trivially integrates them itself by multiplying with the variable
definition range.

However, if the integration range is a subrange of the variable range,
this was not considered. This resulted in wrong results. for integrals
like `pdf.createIntegral(x, "subrange")`, where the pdf doesn't depend
on x. These kind of integrals can occur in the projections that the
RooAddPdf does, so it's important that they work, and fixing this
partially addresses #11486.

This change also fixes a so-far unknown bug in the `RooXYChi2Var`, which
also used these kind of integrals. Without this fix, the `Integrate()`
feature for `chi2FitTo()` was completely broken, which can be seen in
the output of the `rf609` tutorial with any previous ROOT version. The
tutorial looks okay by chance, because the function is dominted by the
quadratic term in `x` that is constant in the fit. But if one makes this
a floating parameter, the problem gets obvious.

Probably that was the reason why the main model parameter was set
constant to begin with, to sweep the bug under the rug. Now, the
tutorials are updated to have the quadratic coefficient floating too.
And also `stressRooFit`, since the reference file has to be updated
anyway.

To demonstrate that things work correctly now, a new unit test was
implemented that does the closure check of the `integrate()` feature of
the `RooXYChi2Var` with a linear fit function.
guitargeek added a commit to guitargeek/root that referenced this issue Jan 12, 2023
The `RooRealIntegral` class is smart enough to figure out which
variables the function the integrated function doesn't depend on and
trivially integrates them itself by multiplying with the variable
definition range.

However, if the integration range is a subrange of the variable range,
this was not considered. This resulted in wrong results. for integrals
like `pdf.createIntegral(x, "subrange")`, where the pdf doesn't depend
on x. These kind of integrals can occur in the projections that the
RooAddPdf does, so it's important that they work, and fixing this
partially addresses root-project#11486.

This change also fixes a so-far unknown bug in the `RooXYChi2Var`, which
also used these kind of integrals. Without this fix, the `Integrate()`
feature for `chi2FitTo()` was completely broken, which can be seen in
the output of the `rf609` tutorial with any previous ROOT version. The
tutorial looks okay by chance, because the function is dominted by the
quadratic term in `x` that is constant in the fit. But if one makes this
a floating parameter, the problem gets obvious.

Probably that was the reason why the main model parameter was set
constant to begin with, to sweep the bug under the rug. Now, the
tutorials are updated to have the quadratic coefficient floating too.
And also `stressRooFit`, since the reference file has to be updated
anyway.

To demonstrate that things work correctly now, a new unit test was
implemented that does the closure check of the `integrate()` feature of
the `RooXYChi2Var` with a linear fit function.
guitargeek added a commit that referenced this issue Jan 12, 2023
The `RooRealIntegral` class is smart enough to figure out which
variables the function the integrated function doesn't depend on and
trivially integrates them itself by multiplying with the variable
definition range.

However, if the integration range is a subrange of the variable range,
this was not considered. This resulted in wrong results. for integrals
like `pdf.createIntegral(x, "subrange")`, where the pdf doesn't depend
on x. These kind of integrals can occur in the projections that the
RooAddPdf does, so it's important that they work, and fixing this
partially addresses #11486.

This change also fixes a so-far unknown bug in the `RooXYChi2Var`, which
also used these kind of integrals. Without this fix, the `Integrate()`
feature for `chi2FitTo()` was completely broken, which can be seen in
the output of the `rf609` tutorial with any previous ROOT version. The
tutorial looks okay by chance, because the function is dominted by the
quadratic term in `x` that is constant in the fit. But if one makes this
a floating parameter, the problem gets obvious.

Probably that was the reason why the main model parameter was set
constant to begin with, to sweep the bug under the rug. Now, the
tutorials are updated to have the quadratic coefficient floating too.
And also `stressRooFit`, since the reference file has to be updated
anyway.

To demonstrate that things work correctly now, a new unit test was
implemented that does the closure check of the `integrate()` feature of
the `RooXYChi2Var` with a linear fit function.
omazapa pushed a commit to omazapa/root that referenced this issue Apr 13, 2023
The `RooRealIntegral` class is smart enough to figure out which
variables the function the integrated function doesn't depend on and
trivially integrates them itself by multiplying with the variable
definition range.

However, if the integration range is a subrange of the variable range,
this was not considered. This resulted in wrong results. for integrals
like `pdf.createIntegral(x, "subrange")`, where the pdf doesn't depend
on x. These kind of integrals can occur in the projections that the
RooAddPdf does, so it's important that they work, and fixing this
partially addresses root-project#11486.

This change also fixes a so-far unknown bug in the `RooXYChi2Var`, which
also used these kind of integrals. Without this fix, the `Integrate()`
feature for `chi2FitTo()` was completely broken, which can be seen in
the output of the `rf609` tutorial with any previous ROOT version. The
tutorial looks okay by chance, because the function is dominted by the
quadratic term in `x` that is constant in the fit. But if one makes this
a floating parameter, the problem gets obvious.

Probably that was the reason why the main model parameter was set
constant to begin with, to sweep the bug under the rug. Now, the
tutorials are updated to have the quadratic coefficient floating too.
And also `stressRooFit`, since the reference file has to be updated
anyway.

To demonstrate that things work correctly now, a new unit test was
implemented that does the closure check of the `integrate()` feature of
the `RooXYChi2Var` with a linear fit function.
@guitargeek
Copy link
Contributor Author

@guitargeek
Copy link
Contributor Author

The underlying problem is that multi-range integrals in the RooProdPdf don't work:

  // Define observables x,y
  RooRealVar x("x","x",-10,10) ;
  RooRealVar y("y","y",-10,10) ;

  // Construct the background pdf (flat in x,y)
  RooUniform px("px","px",x) ;
  RooUniform py("py","py",y) ;
  RooProdPdf bkg("bkg","bkg",px,py) ;

  // Construct the SideBand1,SideBand2,Signal regions
  //
  //                    |
  //      +-------------+-----------+
  //      |             |           |
  //      |    Side     |   Sig     |
  //      |    Band1    |   nal     |
  //      |             |           |
  //    --+-------------+-----------+--
  //      |                         |
  //      |           Side          |
  //      |           Band2         |
  //      |                         |
  //      +-------------+-----------+
  //                    |

  x.setRange("SB1",-10,+10) ;
  y.setRange("SB1",-10,0) ;

  x.setRange("SB2",-10,0) ;
  y.setRange("SB2",0,+10) ;

  x.setRange("SIG",0,+10) ;
  y.setRange("SIG",0,+10) ;

  x.setRange("FULL",-10,+10) ;
  y.setRange("FULL",-10,+10) ;

  RooArgSet deps{x, y};

  bkg.setNormRange("SB1,SB2");

  RooArgSet normSet{x, y};

  // Doesn't work
  std::cout << bkg.getVal(normSet) << std::endl;

@guitargeek guitargeek modified the milestones: 6.34.00, 6.34/00 Apr 24, 2024
@guitargeek guitargeek removed this from the 6.34.00 milestone Sep 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant