[RF] Fixes and improvements to RooTruthModel and related tutorials
#12180
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
guitargeek
requested review from
Axel-Naumann,
couet,
hageboeck and
lmoneta
as code owners
guitargeek
force-pushed
the
anaconv_1
branch
2 times, most recently
from
8e0154d to
2f39809
Compare
|
Starting build on |
|
Build failed on ROOT-performance-centos8-multicore/cxx17. Errors:
Failing tests: |
The RooTruthModel can't be used in that test, because it will always return zero for negative observable values. The point of the tutorial is to demostrate that the fit can deal with negative amplitude values for negative observable values, so this feature of the RooTruthModel is not helpful here. It was only a coincidence that the tutorial still worked: there was a typo in the RooFormulaVars (extra whitespace), causing the RooTruthModel to just relay to the original formula instead of using the compiled code that returns zero for negative observable values.
The `RooTruthModel` when compiled for a basis registered only the `RooFormulaVar` for the basis as a value server, instead of replicating the server structore of the `RooFormulaVar` as done in the general `RooResolutionModel`. While this was the correct thing to do for the "generic basis" case where the RooFormulaVar was actually evaluated, it was wrong for the case where it is not evaluated (any other case). This commit changes the `RooTruthModel::changeBasis()` function such that in the non-generic basis case, it will take over the servers of the `RooFormulaVar`, just like in the implementation of the `RooResolutionModel` base class. This change is done to avoid unnecessary evaluation and copying in the RooFit GPU BatchMode. Also, a memory leak is fixed where the old basis was not deleted in case the resolution model owned the basis.
It is overly pedantic to ask from the user to pass the formula expression without whitespaces, therefore whitespaces are now removed from the input before matching the basis functions.
|
Starting build on |
Contributor
Author
|
Sorry, I made a typo in the tutorials myself now... I have updated the PR one more time, it should work now |
This was referenced Mar 31, 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.
rf704_aplitudefittutorialMore detail in the commit descriptions.