[RF] Explicitly set empty normalization set in RooGenProdProj
#12010
Conversation
This file contains hidden or 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
Since the change to Asimov datasets in `testTestStatistics`, there are warnings that tell you to explicitly set `SumW2Error()` because you are fitting to weighted data. That's what is done in this commit. This means the message level threshold can we changed also to not silence warnings anymore.
In `RooAbsArg::redirectServers()`, we don't need to do anything if there are no new servers or if the only new server is this RooAbsArg itself. And by returning early, we avoid potentially annoying side effects of the redirectServersHook.
The idea of the RooGenProdProj is that we divide two integral objects each created with this makeIntgral() function to get the normalized integral of a product. Therefore, we don't need to normalize the numerater and denominator integrals themselves. Doing the normalization would be expensive and it would cancel out anyway. However, if we don't specify an explicit normalization integral in createIntegral(), the last-used normalization set might be used to normalize the pdf, resulting in redundant computations. For this reason, the normalization set of the integrated pdfs is fixed to an empty set in this case. Note that in RooFit, a nullptr normalization set and an empty normalization set is not equivalent. The former implies taking the last-used normalization set, and the latter means explicitly no normalization. This fixes the performance regression reported in root-project#11814, and a new unit test is implemented to make sure no new numeric integrals pop up in the reproducer code to that issue. Unfortunately, this change means that there will be again warnings about missing `RooAddPdf` normalization sets, but this is not a regression because these warnings only got fixed in the 6.28 development cycle in 290b478.
|
Starting build on |
Contributor
Author
|
@phsft-bot build with flags -DCTEST_TEST_EXCLUDE_NONE=On |
|
Starting build on |
52 tasks
|
Build failed on ROOT-debian10-i386/soversion. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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 idea of the RooGenProdProj is that we divide two integral objects
each created with this makeIntgral() function to get the normalized
integral of a product. Therefore, we don't need to normalize the
numerater and denominator integrals themselves. Doing the normalization
would be expensive and it would cancel out anyway. However, if we don't
specify an explicit normalization integral in createIntegral(), the
last-used normalization set might be used to normalize the pdf,
resulting in redundant computations.
For this reason, the normalization set of the integrated pdfs is fixed
to an empty set in this case. Note that in RooFit, a nullptr
normalization set and an empty normalization set is not equivalent. The
former implies taking the last-used normalization set, and the latter
means explicitly no normalization.
This fixes the performance regression reported in #11814, and a new unit
test is implemented to make sure no new numeric integrals pop up in the
reproducer code to that issue.
Unfortunately, this change means that there will be again warnings about
missing
RooAddPdfnormalization sets, but this is not a regressionbecause these warnings only got fixed in the 6.28 development cycle in
290b478.
This PR also includes some other minor improvements explained in the commit descriptions.