-
Notifications
You must be signed in to change notification settings - Fork 1.3k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[RF] Various flaws in the RooLagrangianMorphFunc #9845
Closed
11 tasks done
Labels
Comments
2 tasks
2 tasks
Issue opened again because the points raised in this GitHub issue were only partially addressed by the PR. |
guitargeek
added a commit
to guitargeek/root
that referenced
this issue
Dec 14, 2022
This is to avoid having to do hacks with the `RooStringVar`. Closes root-project#9845.
guitargeek
added a commit
to guitargeek/root
that referenced
this issue
Dec 14, 2022
This is to avoid having to do hacks with the `RooStringVar`. Closes root-project#9845.
guitargeek
added a commit
to guitargeek/root
that referenced
this issue
Dec 15, 2022
This is to avoid having to do hacks with the `RooStringVar`. Closes root-project#9845.
guitargeek
added a commit
to guitargeek/root
that referenced
this issue
Dec 15, 2022
This is to avoid having to do hacks with the `RooStringVar`. The RooLagrangianMorphFunc constructor that was used exclusively by the factory interface can also be removed now. Closes root-project#9845.
guitargeek
added a commit
that referenced
this issue
Dec 15, 2022
This is to avoid having to do hacks with the `RooStringVar`. The RooLagrangianMorphFunc constructor that was used exclusively by the factory interface can also be removed now. Closes #9845.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
While preparing another PR, I noticed various flaws with the RooLagrangianMorphFunc that need to be fixed by someone who is more familiar with this class, like @rahulgrit:
.cxx
source (e.g.get()
, the other overload ofget()
, andaddCouplings()
). If they are unused in the remaining file and not part of the public interface, why not remove them?setup()
function is called in the class constructor when_diagrams
is still empty. So thediagrams.size() > 0
check will never be true and the if-block can be removed, no?_nonInterferig
data member is used in a few member functions, but it is never filled so always an empty vector. How meaningful is that? Can_nonInterfering
be removed?this
, just to access a class member that is already marked as a class member with the underscore prefix convention. Please replace all occurences ofthis->_
in the source file with just_
.makeCrosssectionContainer()
returns aTPair*
that is owned by the caller. Please returnstd::unique_ptr<TPair>
is this case, or even better return astd::pair
instead (avoid using old ROOT container classes that were written before the standard library was introduced).createTH1
returns an owning raw pointer too, while it should return astd::unique_ptr<TH1>
in this case. Can this interface still be changed without disrupting users too much? No, this can break usercode, so this suggestion was not implemented!All these flaws above have been addressed by @rahulgrit in #9912.
Some more flaws were fixed in #11023.
getCache
function takes a unused argument. It is a private function, so to signature can simply be changed and the argument removed_ownsParameters
member should be removedHowever, that's not all! There are more issues with this class that have to be resolved in a followup PR:
new
in the file and you will find them, almost everything created on the heap is leaking.RooStringVar
is used in the configuration of the class. We should investigate if it's not possible to simply usestd::string
here.The text was updated successfully, but these errors were encountered: