[RF] Improve implementation of RooAbsArg::redirectServers()
#12040
Conversation
|
Starting build on |
guitargeek
force-pushed
the
replaceServer
branch
from
ca6150e to
441af64
Compare
|
Starting build on |
1 similar comment
|
Starting build on |
guitargeek
changed the title
RooAbsArg::replaceServer() saferRooAbsArg::redirectServers()
guitargeek
force-pushed
the
replaceServer
branch
from
17ba81e to
3bdb3bd
Compare
|
Starting build on |
The `RooAbsArg::replaceServer()` function is quite dangerous to use,
because it leaves your RooFit objects in an invalid state. See for
example this code:
```
RooRealVar x("x", "x", -10, 10);
RooRealVar mean("mean", "mean of gaussian", 1, -10, 10);
RooRealVar sigma("sigma", "width of gaussian", 1, 0.1, 10);
RooGaussian gauss("gauss", "gaussian PDF", x, mean, sigma);
RooRealVar mean2(mean);
gauss.replaceServer(mean, mean2, true, false);
gauss.Print("v");
std::cout << "x : " << &gauss.getX() << std::endl;
std::cout << "mean : " << &gauss.getMean() << std::endl;
std::cout << "sigma: " << &gauss.getSigma() << std::endl;
```
Here, the proxy for `mean` will still point to the original `mean`, but
the server was redirected to the copy `mean2`. This is dangerous, and
desyncing of the proxy and server list are actually the underlying
reason for a set of RooFit problems.
The safter `RooAbsArg::redirectServers()` should always be used,
becauese that one is also updating the proxies. Therefore, the
`replaceServer()` interface is now marked as dangerous everywhere
possible: in a printout when you use it, in the docs, and with the
`R__SUGGEST_ALTERNATIVE` macro.
Internally, the `replaceServer()` was also used in `redirectServers()`.
But this was also causing problems: `replaceServer()` always adds the
new server at the end of the server list, which means the list gets
reordered. This can confuse usercode that rely on the server list being
ordered (yes, that's not a good idea anyway, but there are many codes
that do this). This reordering can also be seein in the example code
above.
Therefore, the `redirectServers()` function is now rewritten to replace
the server without changing its position in the server list. This also
means that the original server list doesn't need to be copied, as not
iterators are invalidated.
Furthermore, the `redirectServers()` is more optimized now. Before, it
redundantly figured out whether a server was a value and/or shape
server. Now, this is figured out only once when removing the original
server from the client.
In summary: this PR makes RooFit code safer and faster by changing
`RooAbsArg::redirectServers()`.
It doesn't make sense to clear the servers in the beginning of the constructor when no servers were adeed yet.
guitargeek
force-pushed
the
replaceServer
branch
from
3bdb3bd to
b8cc76d
Compare
|
Starting build on |
|
Yes you are right! More updates to the docs will come later for sure: this is not the last PR I intend to make for the client-server stuff. |
3 participants
The
RooAbsArg::replaceServer()function is quite dangerous to use,because it leaves your RooFit objects in an invalid state. See for
example this code:
Here, the proxy for
meanwill still point to the originalmean, butthe server was redirected to the copy
mean2. This is dangerous, anddesyncing of the proxy and server list are actually the underlying
reason for a set of RooFit problems.
The safter
RooAbsArg::redirectServers()should always be used,becauese that one is also updating the proxies. Therefore, the
replaceServer()interface is now marked as dangerous everywherepossible: in a printout when you use it, in the docs, and with the
R__SUGGEST_ALTERNATIVEmacro.Internally, the
replaceServer()was also used inredirectServers().But this was also causing problems:
replaceServer()always adds thenew server at the end of the server list, which means the list gets
reordered. This can confuse usercode that rely on the server list being
ordered (yes, that's not a good idea anyway, but there are many codes
that do this). This reordering can also be seein in the example code
above.
Therefore, the
redirectServers()function is now rewritten to replacethe server without changing its position in the server list. This also
means that the original server list doesn't need to be copied, as not
iterators are invalidated.
Furthermore, the
redirectServers()is more optimized now. Before, itredundantly figured out whether a server was a value and/or shape
server. Now, this is figured out only once when removing the original
server from the client.
In summary: this PR makes RooFit code safer and faster by changing
RooAbsArg::redirectServers().