[RF] Avoid using RooAbsArg::redirectServer() in RooFit
#12129
Merged
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 commit 52ff06a, the usage of `replaceServer()` is discouraged because it's too easy to forget to replace the proxy arguments as well, resulting in de-syncing of proxies and servers. To avoid the warning that is now printed when using `replaceServer` in regular RooFit code, the usage of `replaceServer` in combination with `RooRealProxy::setArg()` in RooAbsAnaConvPdf is replaced with `redirectServers()`.
The implementation of `HistFactoryNavigation::ReplaceNode` was wrong, which I spotted because it was the remaining place in RooFit where the dangerous `RooAbsArg::replaceServer()` was used. It didn't update the proxies accordingly, resulting in proxy-server-desync, and it also didn't set the correct value and shape server properties (it just hardcoded them both to `false` for the new server instead of taking them over from the old server).
|
Starting build on |
lmoneta
approved these changes
Jan 30, 2023
Member
lmoneta
left a comment
lmoneta
left a comment
There was a problem hiding this comment.
LGTM!
Thank you Jonas for these fixes!
This was referenced Jan 30, 2023
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.
This is a follow-up to #12040.
The
RooAbsArg::redirectServer()method is dangerous and should not be used directly. It was for example used wrongly in HistFactory. In the RooAbsAnaConvPdf is was actually used correctly, but it's still better to useredirectServers()consistently, which avoids the new warnings you get when usingredirectServer().More detail in the commit description.
Also includes a commit that irons over RooFit with
clang-tidy, adding some missing overrides.