[RF] Fix weights in SumW2Error() fits to RooDataHist with batch mode#9161
[RF] Fix weights in SumW2Error() fits to RooDataHist with batch mode#9161guitargeek merged 3 commits intoroot-project:masterfrom
SumW2Error() fits to RooDataHist with batch mode#9161Conversation
guitargeek
requested review from
bellenot,
hageboeck and
lmoneta
as code owners
|
Starting build on |
lmoneta
left a comment
lmoneta
left a comment
There was a problem hiding this comment.
I have just a comment on the new FItResult function
|
Build failed on mac11.0/cxx17. Failing tests: |
If one compares RooFitResults that were produced with external covariance matrices (e.g. from fits with the `SumW2Error()` option), there will be segfaults in the covariance matrix comparison part of `RooFitResult::isIdentical`. Since it's often not important to compare the covariance matrices, a new function `RooFitResult::isIdenticalNoCov` is introduced to compare only the parameters and their uncertainties. The uncertainties are inferred from the covariance matrix after all, so if there is something wrong with the covariance matrix one will still catch it indirectly via the parameter uncertainties.
If the `SumW2Error` correction is requested on fits with the batch mode, the weights of each entry are currently determined to be the square of the original weight for each entry. In the binned fit case, this is wrong. One should not square the entry weight (or more precisely bin weight in the binned case), but take the sum of squares of event weights when they were added to the histrogram. The RooDataHist class keeps track of these sum of squared weights, and this commit changes the code in `RooNLLVar` and the RooFit dataset classes such that these sums of squared weights are used in the batch mode fits. This fixes issue root-project#9118.
This unit test was based on the code that was used to reproduce GitHub issue root-project#9118.
guitargeek
force-pushed
the
issue-9118
branch
from
0ce24ec to
4ee6d62
Compare
|
Starting build on |
|
Fixed the documentation of |
|
Build failed on mac11.0/cxx17. Failing tests: |
|
Build failed on windows10/cxx14. |
| const RooArgList& randomizePars() const; | ||
|
|
||
| Bool_t isIdentical(const RooFitResult& other, Double_t tol=1e-6, Double_t tolCorr=1e-4, Bool_t verbose=kTRUE) const ; | ||
| bool isIdenticalNoCov(const RooFitResult& other, double tol=1e-6, bool verbose=true) const ; |
There was a problem hiding this comment.
I am not sure if adding this function in FitResult is really needed there. It could be maybe added in some utility class.
There was a problem hiding this comment.
So you would put this into RooHelpers.h? In my opinion, it is justified here to have isIdenticalNoCov as a class member, because isIdentical is also a class member and isIdentical uses isIdenticalNoCov because it's the first part of the check. Like this, both the parameter and the covariance matrix checking code are in RooFitResult.cxx. If isIdenticalNoCov would be in RooHelpers, the parameter and covariance matrix checking code would be in two different files, causing further complications: they both use a isIdenticalErrMsg helper function, and if the code is not in the same source file anymore I can't put this helper function in the anonymous namespace anymore.
3 participants
If the
SumW2Errorcorrection is requested on fits with the batch mode,the weights of each entry are currently determined to be the square of
the original weight for each entry.
In the binned fit case, this is wrong. One should not square the entry
weight (or more precisely bin weight in the binned case), but take the
sum of squares of event weights when they were added to the histrogram.
The RooDataHist class keeps track of these sum of squared weights, and
this commit changes the code in
RooNLLVarand the RooFit datasetclasses such that these sums of squared weights are used in the batch
mode fits.
This fixes issue #9118.