Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[RF] Issue with RooSimultaneous in 6.24.00 ? #8307

Closed
JBdeVivie opened this issue Jun 1, 2021 · 1 comment · Fixed by #8364 or #8372
Closed

[RF] Issue with RooSimultaneous in 6.24.00 ? #8307

JBdeVivie opened this issue Jun 1, 2021 · 1 comment · Fixed by #8364 or #8372
Assignees
@guitargeek
Labels
affects:master affects:6.24 bug in:RooFit/RooStats

Comments

@JBdeVivie
Copy link

Hello.

I noticed a weird feature in root 6.24.00, when trying to fit a model using a RooSimultaneous pdf.

Am I doing something forbidden in root 6.24.00 ?

Best,

Jean-Baptiste

Describe the bug

I have a model with a single category, describing the shape of a distribution, with three unconstrained nuisance parameters, one constrained nuisance parameter and one parameter of interest. The three unconstrained NP describe the shape of the background and the background yield. The poi is proportional to the signal yield. The dataset is a background only dataset.

I can do the fit with as pdf : model_had1A = RooProdPdf (fsb_had1A, constbias_had1A) where
fsb = ns x fs + nb x fb, fs and fb are signal and background pdf, ns contains the poi, nb is free floating. It runs
smoothly even if we are close to the unphysical region (poi < 0). (Probably many error messages from the exploration of the unphysical region have been removed from the output.)
I can also try a fit with the pdf : simPdf = RooSimultaneous (indexCat=had1A, had1A=model_had1A) : this is virtually the same pdf, but this time embeded in a RooSimultaneous object. The fit fails.

Between the two root versions, one things that appeared weird to me is that fsb_had1A does not seem to be
normalized when embeded in the RooSimultaneous in root 6.24.00. Maybe that is fine, but this is different from
what I see in root 6.22.02.

Expected behavior

I would expect exactly the same results in both fits, with a best fit poi = 0 and a reasonable uncertainty.
This is what I see in root-6.22.02. In root 6.24.00 the fit with a RooSimultaneous fails.

To Reproduce

I put the code here /afs/cern.ch/user/j/jdevivi/public/ISSUEROOFIT

In root-6.24.00, I just do
root.exe testWSsimulvsprod.C

In root-6.22.02, I do
root.exe load.C testWSsimulvsprod.C
since I use a RooCrystalBall from root-6.24 and did not put the code in the workspace.

Log files can be found in the same directory.

Setup

ROOT version : 6.24.00
Operating system : macos 10.15.7 clang-12
I built root myself. But I also tried via a docker image built for ATLAS
@JBdeVivie JBdeVivie added the bug label Jun 1, 2021
guitargeek added a commit to guitargeek/root that referenced this issue Jun 7, 2021
@guitargeek
[RF] Handle nullptr or empty norm set ambiguity in RooAbsReal::getVal
24a85a1 
The downsteam code, -- like RooAddPdf::getValV for example -- assume
that a nullptr is passed when no normalization is requested. The case of
an empty norm set is not handled correctly in RooAddPdf::getValV,
leading to wrong results.

However, some calling code passes an empty norm set to
RooAbsReal::getVal instead of a `nullptr` in an attempt to disable
normalization.

This commit suggests to solve this ambiguity at the highest possible
level: right at the beginning of RooAbsReal::getVal. If the
normalization set is empty, the pointer pointing to it will be set to
`nullptr`.

This fixes issue root-project#8307.
guitargeek added a commit to guitargeek/root that referenced this issue Jun 7, 2021
@guitargeek
[RF] Add unit test for RooSimultaneous that covers root-project#8307
b771e11
@guitargeek guitargeek mentioned this issue Jun 7, 2021
Merged
@guitargeek guitargeek linked a pull request Jun 7, 2021 that will close this issue
[RF] Handle nullptr or empty norm set ambiguity in RooAbsReal::getVal #8364
Merged
@guitargeek
Copy link
Contributor

Thanks a lot for this bug report!

I propose a solution to this problem in #8364.

guitargeek added a commit that referenced this issue Jun 8, 2021
@guitargeek
[RF] Handle nullptr or empty norm set ambiguity in RooAbsReal::getVal
cff4b09 
The downsteam code, -- like RooAddPdf::getValV for example -- assume
that a nullptr is passed when no normalization is requested. The case of
an empty norm set is not handled correctly in RooAddPdf::getValV,
leading to wrong results.

However, some calling code passes an empty norm set to
RooAbsReal::getVal instead of a `nullptr` in an attempt to disable
normalization.

This commit suggests to solve this ambiguity at the highest possible
level: right at the beginning of RooAbsReal::getVal. If the
normalization set is empty, the pointer pointing to it will be set to
`nullptr`.

This fixes issue #8307.
guitargeek added a commit that referenced this issue Jun 8, 2021
@guitargeek
[RF] Add unit test for RooSimultaneous that covers #8307
2029dec
guitargeek added a commit to guitargeek/root that referenced this issue Jun 8, 2021
@guitargeek
[RF] Handle nullptr or empty norm set ambiguity in RooAbsReal::getVal
2875e21 
The downsteam code, -- like RooAddPdf::getValV for example -- assume
that a nullptr is passed when no normalization is requested. The case of
an empty norm set is not handled correctly in RooAddPdf::getValV,
leading to wrong results.

However, some calling code passes an empty norm set to
RooAbsReal::getVal instead of a `nullptr` in an attempt to disable
normalization.

This commit suggests to solve this ambiguity at the highest possible
level: right at the beginning of RooAbsReal::getVal. If the
normalization set is empty, the pointer pointing to it will be set to
`nullptr`.

This fixes issue root-project#8307.
guitargeek added a commit to guitargeek/root that referenced this issue Jun 8, 2021
@guitargeek
[RF] Add unit test for RooSimultaneous that covers root-project#8307
2689776
guitargeek added a commit to guitargeek/root that referenced this issue Jun 8, 2021
@guitargeek
[RF] Handle nullptr or empty norm set ambiguity in RooAbsReal::getVal
cc964c3 
The downsteam code, -- like RooAddPdf::getValV for example -- assume
that a nullptr is passed when no normalization is requested. The case of
an empty norm set is not handled correctly in RooAddPdf::getValV,
leading to wrong results.

However, some calling code passes an empty norm set to
RooAbsReal::getVal instead of a `nullptr` in an attempt to disable
normalization.

This commit suggests to solve this ambiguity at the highest possible
level: right at the beginning of RooAbsReal::getVal. If the
normalization set is empty, the pointer pointing to it will be set to
`nullptr`.

This fixes issue root-project#8307.
guitargeek added a commit to guitargeek/root that referenced this issue Jun 8, 2021
@guitargeek
[RF] Add unit test for RooSimultaneous that covers root-project#8307
9509f18
guitargeek added a commit to guitargeek/root that referenced this issue Jun 8, 2021
@guitargeek
[RF] Handle nullptr or empty norm set ambiguity in RooAbsReal::getVal
17d2499 
The downsteam code, -- like RooAddPdf::getValV for example -- assume
that a nullptr is passed when no normalization is requested. The case of
an empty norm set is not handled correctly in RooAddPdf::getValV,
leading to wrong results.

However, some calling code passes an empty norm set to
RooAbsReal::getVal instead of a `nullptr` in an attempt to disable
normalization.

This commit suggests to solve this ambiguity at the highest possible
level: right at the beginning of RooAbsReal::getVal. If the
normalization set is empty, the pointer pointing to it will be set to
`nullptr`.

This fixes issue root-project#8307.
guitargeek added a commit to guitargeek/root that referenced this issue Jun 8, 2021
@guitargeek
[RF] Add unit test for RooSimultaneous that covers root-project#8307
d6c7f96
@guitargeek guitargeek mentioned this issue Jun 8, 2021
Merged
@guitargeek guitargeek linked a pull request Jun 8, 2021 that will close this issue
[v624][RF] Handle nullptr or empty norm set ambiguity in RooAbsReal::getVal #8372
Merged
guitargeek added a commit that referenced this issue Jun 8, 2021
@guitargeek
[RF] Handle nullptr or empty norm set ambiguity in RooAbsReal::getVal
93f1421 
The downsteam code, -- like RooAddPdf::getValV for example -- assume
that a nullptr is passed when no normalization is requested. The case of
an empty norm set is not handled correctly in RooAddPdf::getValV,
leading to wrong results.

However, some calling code passes an empty norm set to
RooAbsReal::getVal instead of a `nullptr` in an attempt to disable
normalization.

This commit suggests to solve this ambiguity at the highest possible
level: right at the beginning of RooAbsReal::getVal. If the
normalization set is empty, the pointer pointing to it will be set to
`nullptr`.

This fixes issue #8307.
guitargeek added a commit that referenced this issue Jun 8, 2021
@guitargeek
[RF] Add unit test for RooSimultaneous that covers #8307
6fd928f
pzhristov pushed a commit to alisw/root that referenced this issue Aug 27, 2021
@guitargeek
[RF] Handle nullptr or empty norm set ambiguity in RooAbsReal::getVal
bb0d92f 
The downsteam code, -- like RooAddPdf::getValV for example -- assume
that a nullptr is passed when no normalization is requested. The case of
an empty norm set is not handled correctly in RooAddPdf::getValV,
leading to wrong results.

However, some calling code passes an empty norm set to
RooAbsReal::getVal instead of a `nullptr` in an attempt to disable
normalization.

This commit suggests to solve this ambiguity at the highest possible
level: right at the beginning of RooAbsReal::getVal. If the
normalization set is empty, the pointer pointing to it will be set to
`nullptr`.

This fixes issue root-project#8307.
pzhristov pushed a commit to alisw/root that referenced this issue Aug 27, 2021
@guitargeek
[RF] Add unit test for RooSimultaneous that covers root-project#8307
e2e2d32
@guitargeek guitargeek changed the title Issue with RooSimultaneous in 6.24.00 ? [RF] Issue with RooSimultaneous in 6.24.00 ? Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@guitargeek guitargeek

Labels
affects:master affects:6.24 bug in:RooFit/RooStats
Projects
None yet
Milestone
No milestone
2 participants
@guitargeek @JBdeVivie