[RF] Don't keep the resolution model as a server of RooAbsAnaConvPdf#22621
Draft
guitargeek wants to merge 1 commit into
Draft
[RF] Don't keep the resolution model as a server of RooAbsAnaConvPdf#22621guitargeek wants to merge 1 commit into
guitargeek wants to merge 1 commit into
Conversation
The resolution model passed to a RooAbsAnaConvPdf (RooDecay, RooBDecay,
...) is only a *configuration* object: it specifies which model to
convolve the basis functions with, and the pdf evaluates its own internal
convolutions, not the model itself. Yet it was kept as a (non-value,
non-shape) server, which polluted the computation graph and dragged the
model into RooWorkspace imports and HS3/JSON exports for no reason. It
also broke the HS3 round-trip, since the internal convolution objects
leaked into the JSON with non-conforming names.
The model is now an owned, non-server member instead. This deliberately
mirrors how RooResolutionModel already manages its basis function: a
raw owned pointer (_basis / _ownBasis) that is not a server and whose
redirection is forwarded in redirectServersHook(). See
RooResolutionModel.{h,cxx}. The HS3 exporter now lists its real
dependents explicitly (t, tau, model) instead of auto-exporting servers.
This follows up on 478ad2b, which noted that the model server could
not simply be removed without a server/proxy desync; removing the proxy
entirely avoids that desync by construction.
Schema evolution from class version 3 to 4 is handled with the standard
RooFit mechanism: a versioned `#pragma read` rule in LinkDef.h recovers
the model pointer from the old RooRealProxy, and a RooAbsAnaConvPdf::
ioStreamerPass2() override severs the stale server link once the graph is
live. This is the same two-stage pattern already used for the v5->v6
proxy-list migration (RooAbsArg::ioStreamerPass2) and for the read rules
of RooProduct / RooSuperCategory in roofitcore/inc/LinkDef.h. A unit test
reads back a v3 workspace fixture and checks the server structure,
getModel() and the evaluated values.
Closes root-project#20245.
🤖 Done with the help of [Claude Code](https://claude.com/claude-code) (Claude Opus 4.8)
Test Results 21 files 21 suites 3d 18h 12m 27s ⏱️ For more details on these failures, see this check. Results for commit 9a275ee. |
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
1 participant
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 resolution model passed to a RooAbsAnaConvPdf (RooDecay, RooBDecay, ...) is only a configuration object: it specifies which model to convolve the basis functions with, and the pdf evaluates its own internal convolutions, not the model itself. Yet it was kept as a (non-value, non-shape) server, which polluted the computation graph and dragged the model into RooWorkspace imports and HS3/JSON exports for no reason. It also broke the HS3 round-trip, since the internal convolution objects leaked into the JSON with non-conforming names.
The model is now an owned, non-server member instead. This deliberately mirrors how RooResolutionModel already manages its basis function: a raw owned pointer (_basis / _ownBasis) that is not a server and whose redirection is forwarded in redirectServersHook(). See RooResolutionModel.{h,cxx}. The HS3 exporter now lists its real dependents explicitly (t, tau, model) instead of auto-exporting servers.
This follows up on 478ad2b, which noted that the model server could not simply be removed without a server/proxy desync; removing the proxy entirely avoids that desync by construction.
Schema evolution from class version 3 to 4 is handled with the standard RooFit mechanism: a versioned
#pragma readrule in LinkDef.h recovers the model pointer from the old RooRealProxy, and a RooAbsAnaConvPdf:: ioStreamerPass2() override severs the stale server link once the graph is live. This is the same two-stage pattern already used for the v5->v6 proxy-list migration (RooAbsArg::ioStreamerPass2) and for the read rules of RooProduct / RooSuperCategory in roofitcore/inc/LinkDef.h. A unit test reads back a v3 workspace fixture and checks the server structure, getModel() and the evaluated values.Closes #20245.
🤖 Done with the help of Claude Code (Claude Opus 4.8)