Skip to content
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
guitargeek opened this issue Feb 9, 2022 · 1 comment · Fixed by #9912 or #11899
Closed
11 tasks done

[RF] Various flaws in the RooLagrangianMorphFunc #9845

guitargeek opened this issue Feb 9, 2022 · 1 comment · Fixed by #9912 or #11899

Comments

@guitargeek
Copy link
Contributor

guitargeek commented Feb 9, 2022

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:

  • There are unused free functions only in the .cxx source (e.g. get(), the other overload of get(), and addCouplings()). If they are unused in the remaining file and not part of the public interface, why not remove them?
  • There is an unreachable code branch here. The setup() function is called in the class constructor when _diagrams is still empty. So the diagrams.size() > 0 check will never be true and the if-block can be removed, no?
  • The _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?
  • It's annoying to have all these explicit usage of this, just to access a class member that is already marked as a class member with the underscore prefix convention. Please replace all occurences of this->_ in the source file with just _.
  • makeCrosssectionContainer() returns a TPair* that is owned by the caller. Please return std::unique_ptr<TPair> is this case, or even better return a std::pair instead (avoid using old ROOT container classes that were written before the standard library was introduced).
  • Similarly, createTH1 returns an owning raw pointer too, while it should return a std::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.

  • The getCache function takes a unused argument. It is a private function, so to signature can simply be changed and the argument removed
  • The unused _ownsParameters member should be removed

However, that's not all! There are more issues with this class that have to be resolved in a followup PR:

  • Various memory leaks (search for new in the file and you will find them, almost everything created on the heap is leaking.
    • for example, here we even have some RooDataHists that leak
  • There is a static counter variable in the default constructor. It is unused and should be removed
  • The RooStringVar is used in the configuration of the class. We should investigate if it's not possible to simply use std::string here.
@guitargeek
Copy link
Contributor Author

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
Projects
None yet
1 participant