[RF] RooPolyFunc improvements #11953
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
cburgard
requested review from
guitargeek,
hageboeck and
lmoneta
as code owners
cburgard
force-pushed
the
roopolyfunc-improvements
branch
from
c406335 to
9a16379
Compare
* Add a new constructor and change an existing one slightly to avoid collisions. * Add getters for internal variables. * Fix bugs in printout.
guitargeek
force-pushed
the
roopolyfunc-improvements
branch
from
9a16379 to
2dc3946
Compare
52 tasks
guitargeek
force-pushed
the
roopolyfunc-improvements
branch
from
2dc3946 to
2a0ee54
Compare
guitargeek
force-pushed
the
roopolyfunc-improvements
branch
3 times, most recently
from
68cab5d to
136a3b9
Compare
The `RooPolyFunc::taylorExpand()` had several overloads that were dangerously similar. To avoid user error, the overloads are reduced to a single one that includes all the functionality of the others. This entails an interface change, but it's acceptable because the function has only existed since 6.26 and is used by a few users in ATLAS. Therefore, the benefits of this change outweigh the disadvantages.
guitargeek
force-pushed
the
roopolyfunc-improvements
branch
from
136a3b9 to
cab77a7
Compare
The `RooAbsCollection::snapshot()` overload with an output parameter
should be preferred in the RooFit implementation:
* it doesn't return a pointer that the caller needs to remember to
delete
* the returned pointer doesn't need to be upcasted to the actual type
* less memory allocation on the heap
Like all other RooFit generator context classes, the `RooEffGenContext` should never participate in IO.
guitargeek
force-pushed
the
roopolyfunc-improvements
branch
from
cab77a7 to
736075c
Compare
Contributor
|
@phsft-bot build |
|
Starting build on |
guitargeek
approved these changes
Jan 1, 2023
Contributor
guitargeek
left a comment
guitargeek
left a comment
There was a problem hiding this comment.
LGTM, thank you for the improvement!
The unit tests were failing initially, because your PR effectively made an interface change: if one was passing an integer as the observableValue as the value to extend the function around, it now hit your new overload where the order integer is at the same position as the observaleValue float in the old overload.
To avoid this confusion due to type ambiguity, I took the liberty of going one step further with the interface change and reduced all three overloads of taylorExpand to one, such that there can't be any ambiguity anymore.
I also added some general code improvement commits while at it.
Thanks again for the contribution!
|
Build failed on ROOT-ubuntu18.04/nortcxxmod. Warnings:
And 60 more Failing tests:
|
|
Build failed on ROOT-performance-centos8-multicore/cxx17. Warnings:
And 130 more Failing tests:
|
|
Build failed on mac12/noimt. Warnings:
And 53 more Failing tests:
|
|
Build failed on mac11/cxx14. Warnings:
And 54 more Failing tests:
|
|
Build failed on ROOT-ubuntu2004/python3. Errors:
|
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 Pull request:
Adds more functionality to the RooPolyFunc
Changes or fixes:
Add a new constructor and change an existing one slightly to avoid collisions.
Add getters for internal variables.
Fix bugs in printout.
Checklist:
This PR fixes #