[RF] Add HistFactory preprocess callbacks#22191
Draft
tianzhuli2002 wants to merge 1 commit intoroot-project:masterfrom
Draft
[RF] Add HistFactory preprocess callbacks#22191tianzhuli2002 wants to merge 1 commit intoroot-project:masterfrom
tianzhuli2002 wants to merge 1 commit intoroot-project:masterfrom
Conversation
There was a problem hiding this comment.
Pull request overview
Adds an optional programmatic “preprocess” hook to HistFactory Measurement to let callers create preprocess objects directly on an already-instantiated RooWorkspace, avoiding RooWorkspace::factory()-driven Cling/LLVM issues for very large models.
Changes:
- Introduce
Measurement::PreprocessFunctionCallbackplus APIs to register and apply callbacks during workspace creation. - Execute registered callbacks during
HistoToWorkspaceFactoryFastchannel workspace construction (after existing string-based preprocess calls).
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
roofit/histfactory/src/Measurement.cxx |
Implements callback registration and application logic. |
roofit/histfactory/src/HistoToWorkspaceFactoryFast.cxx |
Invokes preprocess callbacks during per-channel workspace creation. |
roofit/histfactory/inc/RooStats/HistFactory/Measurement.h |
Adds the new callback API and stores callbacks (transient) in Measurement. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+91
to
+96
| void Measurement::AddPreprocessFunctionCallback(PreprocessFunctionCallback callback) | ||
| { | ||
| if (callback) { | ||
| fPreprocessFunctionCallbacks.push_back(callback); | ||
| } | ||
| } |
Comment on lines
+780
to
+783
| /// Programmatic preprocess callbacks, executed during workspace creation. | ||
| /// Transient because std::function is not ROOT-streamable. | ||
| std::vector<PreprocessFunctionCallback> fPreprocessFunctionCallbacks; //! | ||
|
|
Comment on lines
+703
to
+706
| using PreprocessFunctionCallback = std::function<void(RooWorkspace &)>; | ||
| void AddPreprocessFunctionCallback(PreprocessFunctionCallback callback); | ||
| void ApplyPreprocessFunctionCallbacks(RooWorkspace &ws) const; | ||
|
|
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 Pull request is aimed to fix issue #21052 #21052:
Large HistFactory models can create many expression-based preprocess
functions. Constructing these through RooWorkspace::factory can cause crashes during workspace creation due to Cling/LLVM runtime.
Add an optional callback path to Measurement so callers can create
preprocess objects programmatically once the workspace is available.
The existing string-based preprocess function path is unchanged. This is change is also meant to interface with changes to trex-fitter which will try to use this new callback functionality.
Checklist: