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.

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

[RF] Don't skip zero-weight events by default in new BatchMode #11134

Merged
merged 2 commits into from
Aug 8, 2022

Conversation

guitargeek
Copy link
Contributor

The new RooFit batchMode skipped zero-weight events to optimize the
likelihood calculation. However, this should not be done in general,
because it is unexpected to users is the output of batched computations
is not aligned with the original dataset.

This commit also adds a smaller commit with a change to ensure that
norm set args are part of the graph in NormalizationHelpers, also used
in the new BatchMode.

The new RooFit batchMode skipped zero-weight events to optimize the
likelihood calculation. However, this should not be done in general,
because it is unexpected to users is the output of batched computations
is not aligned with the original dataset.
Add the arg from the actual node list in the computation graph. Like
this, we don't accidentally add internal variable clones that the client
args returned. Looking this up is fast because of the name pointer hash
map optimization.

This is done because it will prevent crashes in future RooFit
developments that yet have to be commited.
@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/soversion, ROOT-performance-centos8-multicore/cxx17, ROOT-ubuntu18.04/nortcxxmod, ROOT-ubuntu2004/python3, mac1015/cxx17, mac11/cxx14, windows10/cxx14
How to customize builds

Copy link
Member

@lmoneta lmoneta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants