[RF] Add rf613 tutorial about global observables in RooFit and fixes to bugs discovered when writing the tutorial
#9795
Conversation
|
Starting build on |
lmoneta
left a comment
lmoneta
left a comment
There was a problem hiding this comment.
I have just a small comment, otherwise looks fine to me !
| bool takeGlobalObservablesFromData, | ||
| bool cloneConstraints, | ||
| RooWorkspace * workspace = nullptr); | ||
| RooWorkspace * workspace); |
There was a problem hiding this comment.
Is this needed ? It is a change in the interface for some potential users
There was a problem hiding this comment.
I would have avoided the bug that is fixed in this PR. And fortunately, this change doesn't break any existing code, because RooConstraintSum::createConstraintTerm was only introduced in this release development cycle!
guitargeek
changed the title
rf317 tutorial about global observables in RooFit and fixes to bugs discovered when writing the tutorialrf317 tutorial about global observables in RooFit and fixes to bugs discovered when writing the tutorial
In older releases this was not necessary, because the recursively called `addParameters` was already filling a `RooArgSet`, so the duplicate check happened there. Now the deduplication is only done at the end when the parameters are transferred from a RooArgList to a RooArgSet. That means the adding should be done in silent more to not spam the message logger with harmless "errors".
The `cloneConstraints` parameter was not passed correctly to `RooConstraintSum::createConstraintTerm`, most likely because of a rebase error in the past. To avoid similar errors in the past, the signature of `RooConstraintSum::createConstraintTerm` got changed to not have a default value for the workspace pointer. In this way, interface mismatches will be better caught at compile time.
guitargeek
force-pushed
the
roofit_docs_1
branch
from
0132081 to
b9ffa6a
Compare
|
Starting build on |
|
Last force push was just to fix a typo in the first commit. |
|
Build failed on ROOT-performance-centos8-multicore/default. Failing tests: |
hageboeck
left a comment
hageboeck
left a comment
There was a problem hiding this comment.
Hey, I added quite some comments. Don't feel obliged to address all of them.
| // the negative log-likelihood `l` given the per-event or per-bin | ||
| // observations `x`: | ||
| // | ||
| // l( x | p ) |
There was a problem hiding this comment.
I find this notation confusing. I was going to suggest changing + to *, because I thought that l == likelihood. Maybe you use NLL?
There was a problem hiding this comment.
Ok, now I used L for likelihood all the way, use * and don't talk about negative logarithms.
| // nuisance parameter that is constrained by an auxiliary measurement | ||
| // `lumi_obs` with uncertainty `lumi_obs_sigma`: | ||
| // | ||
| // l'(data | mu, lumi) = l(data | mu, lumi) * Gauss(lumi | lumi_obs, lumi_obs_sigma) |
There was a problem hiding this comment.
- Traditionally, it's
L( obs | pars), but you now didGauss( par | obs, par). The Gaussian is symmetric, but it's not nice. - Now
lis a likelihood again ...
| // l'(data | mu, lumi) = l(data | mu, lumi) * Gauss(lumi | lumi_obs, lumi_obs_sigma) | |
| // l'(data | mu, lumi) = l(data | mu, lumi) * Gauss(lumi_obs | lumi, lumi_obs_sigma) |
| // | ||
| // Since a Gaussian is symmetric in `lumi` and `lumi_obs`, we can | ||
| // reformulate the likelihood to make it apparent that `lumi_obs` is a | ||
| // global observables: |
There was a problem hiding this comment.
| // global observables: | |
| // global observable: |
There was a problem hiding this comment.
I don't see why this would need to be reformulated. Remove this, and directly show the L( obs | par)?
The new tutorial explains: * the concept of global observables * the related command arguments to `RooAbsPdf::fitTo` * how to store global observable values in data
The RooFit convention for constraint terms is to have the constrained parameter also as the mean parameter of the constraint Gaussian, while a so-called global observable (a constant) is put in place of the pdf observable.
guitargeek
force-pushed
the
roofit_docs_1
branch
from
b9ffa6a to
601ef3e
Compare
|
Starting build on |
|
Build failed on ROOT-performance-centos8-multicore/default. Failing tests: |
guitargeek
changed the title
rf317 tutorial about global observables in RooFit and fixes to bugs discovered when writing the tutorialrf613 tutorial about global observables in RooFit and fixes to bugs discovered when writing the tutorial
|
Hi @hageboeck, thanks a lot for your comments! I have addressed all of them, thanks for helping out to get this much needed tutorial for global observables into a good shape 👍 I'll merge now so that this still makes it into the release, but please let me know if you want me to follow up on something. |
|
Build failed on windows10/cxx14. Errors:
|
4 participants
See more details in the commit messages.
This is a followup to #8878, and the first commit silences a harmless error message that appeared after d5c3c58.