[RF] Added offsetting to RooAbsPdf::fitTo
#12001
Conversation
Zeff020
requested review from
guitargeek,
hageboeck and
lmoneta
as code owners
|
Can one of the admins verify this patch? |
|
@phsft-bot build |
|
Starting build on |
guitargeek
changed the title
RooAbsPdf::fitToRooAbsPdf::fitTo
roofit/roofitcore/src/RooAbsPdf.cxx
Outdated
| /// <tr><td> `Offset(std::string const& mode)` <td> Likelihood offsetting mode. Can be either: | ||
| /// - `"none"` (default): no offsetting | ||
| /// - `"initial"`: Offset likelihood by initial value (so that starting value of FCN in minuit is zero). | ||
| /// This can improve numeric stability in simultaneous fits with components with large likelihood values. | ||
| /// Note that this named argument only works if combined with `modularL` or `Parallelize`. For non-modular | ||
| /// likelihoods offsetting is enabled on the likelihood itself. |
There was a problem hiding this comment.
| /// <tr><td> `Offset(std::string const& mode)` <td> Likelihood offsetting mode. Can be either: | |
| /// - `"none"` (default): no offsetting | |
| /// - `"initial"`: Offset likelihood by initial value (so that starting value of FCN in minuit is zero). | |
| /// This can improve numeric stability in simultaneous fits with components with large likelihood values. | |
| /// Note that this named argument only works if combined with `modularL` or `Parallelize`. For non-modular | |
| /// likelihoods offsetting is enabled on the likelihood itself. |
The RooAbsPdf::fitTo() method already has a place where the offsetting is explained, linking to the documentation of createNLL(). Let's not repeat this here.
I would suggest to not change the documentation in this PR, but instead just trying to make the offsetting work in fitTo().
guitargeek
left a comment
guitargeek
left a comment
There was a problem hiding this comment.
Hi @Zeff020, thanks for opening PR!
Have you tested this change? If I try out the code now with the basic Gaussian NLL from the rf101 example (as always), if always errors out:
gauss.fitTo(*data, ModularL(true), Offset(true), Parallelize(true));
Gives:
[#0] ERROR:InputArguments -- RooAbsPdf::createNLL(gauss) ERROR: argument ModularL not allowed in this context
[#0] ERROR:InputArguments -- RooAbsPdf::createNLL(gauss) ERROR: argument ModularL not allowed in this context
[#0] ERROR:InputArguments -- RooAbsPdf::createNLL(gauss) ERROR: illegal combination of arguments and/or missing arguments
terminate called after throwing an instance of 'std::logic_error'
what(): In RooMinimizer constructor: Selected likelihood evaluation but a non-modular likelihood was given. Please supply ModularL(true) as an argument to createNLL for modular likelihoods to use likelihood or gradient parallelization.
And when I don't use Parallelize(true), I even get a segfault.
Do you also see this behavior? If yest, can you fix it please?
|
Hi @guitargeek, my most recent commit should fix the issue that you encountered, at least it does for me locally. |
|
Cool, thanks! Okay I'll try it |
|
@phsft-bot build |
|
Starting build on |
|
Build failed on mac12/noimt. Failing tests:
And 37 more |
|
Build failed on ROOT-performance-centos8-multicore/cxx17. Failing tests:
And 175 more |
|
Hi @Zeff020! There are some tests failing now. Can you check what's going on there? Let me know on Mattermost if you need help |
|
Build failed on ROOT-ubuntu2004/python3. Failing tests:
And 37 more |
|
Build failed on mac11/cxx14. Failing tests:
And 175 more |
Zeff020
force-pushed
the
rooabspdf-offset-fix
branch
from
7b03777 to
85cb30b
Compare
|
Hi @guitargeek, I've amended my most recent commit, could you try the build and tests again? All problems should be resolved now. |
|
@phsft-bot build |
|
Starting build on |
|
Build failed on ROOT-ubuntu18.04/nortcxxmod. Errors:
|
|
Build failed on mac11/cxx14. Failing tests: |
guitargeek
left a comment
guitargeek
left a comment
There was a problem hiding this comment.
LGTM, thanks for the fix! I tried out the fit in the first RooFit tutorial now:
gauss.fitTo(*data, PrintLevel(-1), Save(), ModularL(true), Offset(true), Parallelize(false))->Print();
And I put some printouts in the RooMinimizer to see if the offsetting option is correctly propagated. And it is.
4 participants
This Pull request:
Changes or fixes:
Adds offsetting named argument to
RooAbsPdf::fitTo()as requested in #11856. I have decided to use the naming convention for the named argument options that was already present inRooAbsPdf::CreateNLL()to avoid confusion and for the potential case that the other named argument options in the latter will also be added toRooAbsPdf::fitTo()Checklist:
This PR fixes #