[RF] Fix ranged integrals with factorized variables #12011
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
guitargeek
requested review from
Axel-Naumann,
hageboeck and
lmoneta
as code owners
|
Starting build on |
guitargeek
force-pushed
the
integral_fix_1
branch
from
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.
guitargeek
force-pushed
the
integral_fix_1
branch
from
2b76cfa to
ea62965
Compare
|
Starting build on |
52 tasks
lmoneta
approved these changes
Jan 12, 2023
Member
lmoneta
left a comment
lmoneta
left a comment
There was a problem hiding this comment.
LGTM!
Thanks Jonas for the fix!
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The
RooRealIntegralclass 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
rf609tutorial with any previous ROOT version. Thetutorial looks okay by chance, because the function is dominted by the
quadratic term in
xthat 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
RooXYChi2Varwith a linear fit function.