Repository navigation
[RF] Make RooSimultaneous deal with categorical model parameters - #23142
Merged
Merged
Conversation
A RooSimultaneous can appear as a node inside a compiled compute graph when its index category is not an observable: it then acts as a "switch" that evaluates to the component selected by the current category state, analogous to RooMultiPdf. With an observable index category, the likelihood is decomposed into per-channel terms instead and the RooSimultaneous itself never reaches code generation. Previously, such a node fell through to the generic codegenImpl() fallback, which emits a constant "1.0" and only prints an error. The generated likelihood was silently wrong: plain evaluation through the RooEvaluatorWrapper still used the CPU evaluator and looked fine, but the codegen backend differentiated the wrong function, and with EvalBackend::CodegenNoGrad() the wrong values became directly visible. Emit a nested ternary expression keyed on the actual category state indices, like the RooMultiPdf implementation. The unsupported cases (extendable components, whose scalar evaluation applies a relative yield weight, and derived index categories) now throw a clear error instead of generating wrong code. 🤖 Done with the help of AI
A RooSimultaneous whose index category is not among the observables acts as a "switch" that evaluates to the component selected by the current index state, analogous to RooMultiPdf (see the discussion in GitHub issue root-project#22916). The legacy evaluation backend already handles this correctly, and the discrete-profiling loop in RooMinimizer picks up any floating RooCategory parameter, so such a pdf can be used for discrete profiling when nested inside another model. However, RooSimultaneous::compileForNormSet() unconditionally applied the channel-splitting transformation, which renames the per-channel observables with a "_<channel>_" prefix so that they can be filled from split datasets. For a parameter-index RooSimultaneous there is no splitting by category, the renamed observables never received data, and the CPU backend silently computed wrong likelihood values. Compile like an ordinary self-normalized pdf when no component of the index category is in the normalization set. Together with the previous commit, all three backends now agree with the legacy backend and with the equivalent RooMultiPdf to machine precision, which is covered by the new test. 🤖 Done with the help of AI
Creating a test statistic directly from a RooSimultaneous whose index
category is not among the data columns used to fail on all backends,
because the fitting infrastructure unconditionally tried to split the
dataset by the index category. Such a "switch"-mode RooSimultaneous (see
the previous commit) has no channels to split the data into: the index
category acts as a parameter of the fit, like for RooMultiPdf.
Introduce RooSimultaneous::indexCatIsObservable() as the single
predicate for deciding between the two modes (also used by
compileForNormSet() now), and consult it at every place that assumed
"RooSimultaneous implies channel splitting":
- BatchModeDataHelpers::getDataSpans() ignores the simPdf argument if
there is nothing to split by, covering the data loading of both
RooEvaluatorWrapper::setData() and RooFuncWrapper::loadData()
- the NLL and chi2 construction in FitHelpers, which now build an
ordinary RooNLLVarNew over the full pdf instead of per-channel terms
- the legacy RooAbsTestStatistic constructors, which now pick the
SimMaster mode only for an actual simultaneous fit
With this, a parameter-index RooSimultaneous can be used in place of
RooMultiPdf for the discrete profiling method, both nested inside a
larger model and as the top-level pdf: the NLL values agree with the
NLL of the component pdf selected by the index state, and a fit with a
floating index category profiles the discrete choice like RooMultiPdf
does (without the penalty term, which RooMultiPdf adds on top).
Closes the remaining gap from the discussion in GitHub issue root-project#22916.
🤖 Done with the help of AI
Test Results 23 files 23 suites 3d 16h 26m 18s ⏱️ For more details on these failures, see this check. Results for commit c6953ee. |
lmoneta
approved these changes
Aug 27, 2026
lmoneta
left a comment
lmoneta
left a comment
Member
There was a problem hiding this comment.
LGTM!
Thank you Jonas for implementing this case (categories as model parameters) in the RooSimultaneous pdf
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This PR contains the necessary commits to make the RooSimultaneous work with categorical model parameter, i.e. RooCategory arguments that are not part of the fit dataset.
As a result, the RooSimultaneous behaves now like a RooMultiPdf without the penalty term.