-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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] Fix ranged integrals with factorized variables #12011
Conversation
Starting build on |
caf3fc5
to
2b76cfa
Compare
Starting build on |
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.
2b76cfa
to
ea62965
Compare
Starting build on |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Thanks Jonas for the fix!
The
RooRealIntegral
class is smart enough to figure out whichvariables 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 dependon 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
, whichalso used these kind of integrals. Without this fix, the
Integrate()
feature for
chi2FitTo()
was completely broken, which can be seen inthe output of the
rf609
tutorial with any previous ROOT version. Thetutorial 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 thisa 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 updatedanyway.
To demonstrate that things work correctly now, a new unit test was
implemented that does the closure check of the
integrate()
feature ofthe
RooXYChi2Var
with a linear fit function.