[RF] Constructors RooProdPdf and RooProduct #7809

cburgard · 2021-04-09T08:46:53Z

Explain what you would like to see improved

RooProdPdf and RooProduct have different constructor interfaces.
In a workspace like this:

RooWorkspace w;
w.factory("RooGaussian::a(x[-10,10],0.,1.)")
w.factory("RooGaussian::b(y[-10,10],0.,1.)")

Now, consider this:

w.factory("RooProdPdf::p1({a,b})")
w.factory("RooProduct::p2({x,y})")

p2 does what you think it would.
p1 does not.

On the other hand:

w.factory("RooProdPdf::p3(a,b)")
w.factory("RooProduct::p4(x,y)")

p3 works.
p4 does not.

How it could be improved

The constructor interfaces of these classes should be harmonized.

The text was updated successfully, but these errors were encountered:

guitargeek · 2021-04-09T11:09:01Z

Linking a conversation that is connected to the issue: #7766 (comment)

cburgard · 2021-04-12T10:14:52Z

After a discussion with Wouter, it seems that the constructors of RooProdPdf and RooProduct should all be changed to use RooArgSet instead of RooArgList

This commit fixes three issues: 1. Introduce `RooProduct` constructor from two RooAbsReals to be consistent with the `RooRrodPdf`, which has a constructor from two RooAbsPdfs. 2. Remove a useless dummy constructor for a `RooProdPdf` with no factors, which somehow prevented the constructor from a RooFit collection to be picked up correctly by `RooWorkspace::factory()` 3. Change the interfaces of the RooProduct and RooProdPdf constructors from RooFit collections to take RooArgSets and not RooArgLists, because all factors should be forced to have unique names. This change is completely backwards compatible, because a RooArgSet can be implicitely constructed from a RooArgList, in which case it also does the duplicate checking. A unit test for the issue was also implemented. Closes root-project#7809.

This commit fixes three issues: 1. Introduce `RooProduct` constructor from two RooAbsReals to be consistent with the `RooRrodPdf`, which has a constructor from two RooAbsPdfs. 2. Remove a useless dummy constructor for a `RooProdPdf` with no factors, which somehow prevented the constructor from a RooFit collection to be picked up correctly by `RooWorkspace::factory()` The request to change the RooProduct and RooProdPdf constructors from RooFit collections to take RooArgSets and not RooArgLists was not followed. This would be a breaking change for people that use `RooProduct` to square a function for example. A unit test for the issue was also implemented. Closes root-project#7809.

This commit fixes two issues: 1. Introduce `RooProduct` constructor from two RooAbsReals to be consistent with the `RooRrodPdf`, which has a constructor from two RooAbsPdfs. 2. Remove a useless dummy constructor for a `RooProdPdf` with no factors, which somehow prevented the constructor from a RooFit collection to be picked up correctly by `RooWorkspace::factory()` The request to change the RooProduct and RooProdPdf constructors from RooFit collections to take RooArgSets and not RooArgLists was not followed. This would be a breaking change for people that use `RooProduct` to square a function for example. A unit test for the issue was also implemented. Closes root-project#7809.

This commit fixes two issues: 1. Introduce `RooProduct` constructor from two RooAbsReals to be consistent with the `RooRrodPdf`, which has a constructor from two RooAbsPdfs. 2. Remove a useless dummy constructor for a `RooProdPdf` with no factors, which somehow prevented the constructor from a RooFit collection to be picked up correctly by `RooWorkspace::factory()` The request to change the RooProduct and RooProdPdf constructors from RooFit collections to take RooArgSets and not RooArgLists was not followed. This would be a breaking change for people that use `RooProduct` to square a function for example. A unit test for the issue was also implemented. Closes #7809.

github-actions · 2022-06-23T06:09:05Z

Hi @guitargeek, @lmoneta,