[RF] Several improvements in RooFit chi-square fitting #13651
Merged
Conversation
This file contains 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
force-pushed
the
roofit_chi2
branch
from
bfc3d9c
to
7db7c18
Compare
guitargeek
force-pushed
the
roofit_chi2
branch
from
7db7c18
to
8355399
Compare
guitargeek
requested review from
Axel-Naumann and
vepadulano
as code owners
guitargeek
changed the title
guitargeek
force-pushed
the
roofit_chi2
branch
from
8355399
to
4e44fd6
Compare
Almost a year ago, I fixed the support for comma-separated normalization
ranges for pdfs, e.g. `pdf.setNormRange("range1,range2")` was fixed also
for RooAddPdfs.
As a result, the logic for multi-range likelihood fits was removed from
`createNLL()`, because the multi-range fit didn't have to be treated as
a special case anymore.
The same applies also to chi-square fits. In fact, the reason why the
`RooChi2Var` constructor and `RooAbsReal::createChi2()` methods had a
`RooAbsPdf` overload was *only* this workaround! For regular
RooAbsReals, the workaround was not necessary, because there is no
normalization.
Therefore, quite a few functions were removed in this commit.
The multi-range chi2 fit is now also validated by the multi-range fit
unit test in `testRooAbsPdf`.
This is a follow-up to PR root-project#11455 (commit fa10523 in particular),
where the same change was already make for regular likelihoods.
It can be checked at runtime if a given `RooAbsReal` is a pdf or not. Like this, we also don't need a separate override of `createChi2(RooDataSet &)` in RooAbsPdf.
This is achieved in the same was as in `createNLL()`, by forwarding the configuration options to the `RooAbsOptTestStatistic` base class.
guitargeek
force-pushed
the
roofit_chi2
branch
from
4e44fd6
to
dc693dc
Compare
|
Starting build on |
After adding support for subrange fits with the `RooXYChi2Var`, it initially didn't work because the y-value errors were missing in the dataset. This is because `RooDataSet::reduce()` is not copying the errors over to the reduced dataset, which is a bug. Fortunately, there is a straighforward solution. To create the reduced `RooDataSet`, it now doesn't use some buggy private constructor (that is now removed), but instead the trusty `RooAbsData::emptyClone()` method, for which I fixed the problem with the errors not being copied already some time ago.
|
Build failed on windows10/default. Failing tests: |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 changes in this PR address this forum post...
https://root-forum.cern.ch/t/chi2fito-with-yvar-does-not-support-range-option/56369
...and follows up on the discussion here:
#13638
Short summaries of the commits, more detail in the commit descriptions:
createChi2(), because for a few months already this can be dealt with in the test statistics base classesNumCPU()andRange()in createChi2() for RooDataSetsRooDataSet::reduce()by also copying the stored errors