- Jan 19, 2023
-
-
Axel Naumann authored
See https://github.com/root-project/root/issues/11937 for why `-flat_namespace` is bad. This reverts commit a05d4bed.
-
- Jan 18, 2023
-
-
Jonas Rembser authored
-
Jonas Rembser authored
It's to expensive to do for large graphs, too complicated to implement, an not necessary anymore because also in BatchMode the full computation graph is cloned. This also means that the `RooAbsReal` that is evaluated in `RooAbsReal::getValues()` also needs to be cloned, but this is okay because the interface is so far only used in unit tests and not performance critical. It was actually necessary to do the cloning there to begin with, since the BatchMode was not correctly reverting all modifications to the computation graph anyway.
-
Jonas Rembser authored
In `RooProdPdf::getConstraints()`, there is a place where it checks if the parameters of one component pdf are overlapping with the observables or all parameters of the model. This can get expensive for models with a huge number of parameters. For the ATLAS Higgs combination workspace I have, it is a significant fraction of `createNLL()`, which takes 100 seconds total. We can greatly speed up this check by caching the ordered name pointers of the parameters and observables collections. This makes the time spent in `getConstraints()` negligible for now (5 out of the remaining 80 seconds in `createNLL` for the ATLAS Higgs combination). It was important to optimize this part, because the exact same code is also used in the BatchMode, for which I aim to make `createNLL` much faster than 80 seconds in total.
-
Jonas Rembser authored
The IO of RooFit proxies can be quite fragile sometimes, and it can happen when reading a RooProduct that the `_proxyList` is not synced with the proxy members. In dbc96810, I decided to throw an exception is this case, but I realized this was too strong: too many old workspace are affected. In all cases that I know of, one can simply recover by correctly resetting the `_proxyList`. This is now what is done, and only a warning is printed. The warning includes information on all the proxies, so the user can figure out themselves if what RooFit is doing here is correct.
-
- Jan 17, 2023
-
-
Javier Lopez-Gomez authored
The `target` attribute in a rule specification is mandatory, but the list can be empty. In principle, we allow these rules for now.
-
Jonas Rembser authored
This makes especially the tutorials less verbose.
-
Jonas Rembser authored
A new mechanism is suggested to enable function signatures that accept both references to RooFit arguments, or `double`s that will be implicitly converted to `RooConst&`. The only thing that you need to do is to replace `RooAbsReal&` with `RooAbsReal::Ref`. To test this mechanism, it is now supported for the `RooGaussian`, `RooLandau`, `RooArgusBG`, `RooPoisson`, and `RooExtendedPdf`. The original constructors are also kept, in case people rely on their exact signature with C++ reflection.
-
Axel Naumann authored
The commits in this branch do not pass the test suite by themselves; they are left for historical / informational purposes.
-
Axel Naumann authored
Without, the JIT fails to compile roottest/root/aclic/misc/assertROOT7027.C correctly.
-
Axel Naumann authored
m_JIT.getSymbolAddress() invokes the symbol materializers, which compile (which is sort of okay) but also try autoloading (which totally is not okay). Instead, implement a function to search existing JIT symbols. This can be accelerated by looking up the whole set of symbols, instead of doing it symbol by symbol. I leave that refactoring for later...
-
Axel Naumann authored
-
Axel Naumann authored
Small functions might get inlined, and hiding their definition prevents the inliner from doing its job.
-
Axel Naumann authored
With the upgrade to llvm-13, the JIT lost the ability to re-use existing weak symbols that the JIT had already emitted, instead only looking at dlsym. This causes a significant increase in JITted symbols, and thus a significant slow-down of cling / its JIT. This restores the old behavior, with an identical set of symbols that jet jitted.
-
Jonas Rembser authored
It doesn't make sense to clear the servers in the beginning of the constructor when no servers were adeed yet.
-
Jonas Rembser authored
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()`.
-
Lorenzo Moneta authored
-
Jonas Rembser authored
The member that points to the last normalization set should not take part in IO. Also, a memory leak is fixed and some code is modernized by using `std::unique_ptr`.
-
ferdymercury authored
* whole class is designed for Int_t, not Long64_t prevent overflow with an error message * Apply agheata suggestion To catch more overflow cases
-
- Jan 16, 2023
-
-
Lorenzo Moneta authored
-
Jonas Rembser authored
-
Jonas Rembser authored
With the `RooAbsReal::createIntegral()` function, one can in general create integrals on multiple comma-separated ranges. However, it can happen all too easily in multi-ranged fits that RooFit creates normalization integrals on multiple overlapping ranges: even if for the full set of observables the ranges are non-overlapping, RooFit might internally create integrals with respect to a subset of the observables for which the ranges are overlapping. To avoid these kind of mistakes, `createIntegral()` now throws an exception if the ranges are overlapping for the integration variables, because it doesn't support overlapping ranges. The overlapping domain would be double-counted.
-
Jonas Rembser authored
The recently-introduced `RooHelpers::checkIfRangesOverlap()` function so far was taking a pointer to some RooAbsData object. If the data was a RooDataHist, the range checking didn't use the limits defined by the custom range, but by the custom binning. However, there was no usecase for using the custom binnings, and having this extra argument just made the function more confusing and unconvenient to use. As the function was only introduced in 6.26 and as mostly a RooFit implementation detail it was never advocated to the users, it should be still fine to change the interface.
-
Jonas Rembser authored
The type of the first bool parameter of one `RooTemplateProxy` constructor with the signature `RooTemplateProxy(name, title, owner, bool, bool, bool)` is now templated such that implicit conversion from int or pointers to bool is disabled. This is because there is another constructor with the signature `RooTemplateProxy(name, title, owner, T& ref)`. It happened already more than once that other developers accidentally used a `T*` pointer instead of a reference, in which case it resolved to this constructor via implicit conversion to bool. This is completely meaningless and should not happen.
-
Jonas Hahnfeld authored
-
- Jan 15, 2023
-
-
will-cern authored
A set of, hopefully, non-controversial changes to some RooFit classes to allow certain data members to be accessed publicly. * allow named sets of workspace to be accessed * allow access to arg's workspace * allow access to RooAbsReal's flags to mark a component as selected or not * allow access to test statistic's operMode * allow p-values and associated errors to be set
-
Axel Naumann authored
-
will-cern authored
Explanation is in comments for new methods - allows underlying dataHist to be cloned if necessary to take ownership.
-
- Jan 13, 2023
-
-
Jonas Hahnfeld authored
According to https://github.com/root-project/root/issues/9137, we get bogus data if a friend is shorter than the main tree. The datasetspec test acknowledges this, but still checks the results in the MT case. This miraculously seems to work most of the time, but we sometimes see failures in the nightlies, especially on macOS, so remove it.
-
Florine de Geus authored
-
Florine de Geus authored
-
will-cern authored
This memory leak is demonstrated with the following ROOT macro: ``` { { RooExpensiveObjectCache::instance(); // force the standard instance construction (otherwise created in factory method call) cout << "make ws" << endl; RooWorkspace w("combined", "combined"); cout << "factory method:" << endl; w.factory("RooGaussian::gaus(x[-5,5],mean[0,-5,5],sigma[1,0.1,3])"); w.writeToFile("/tmp/test.root"); cout << "reading back" << endl; { TFile f("/tmp/test.root"); RooWorkspace *w2 = f.Get<RooWorkspace>("combined"); std::cout << "deleting w2" << endl; delete w2; } std::cout << "deleting w" << endl; } } ``` along with a modification to `RooExpensiveObjectCache` to printout when an instance is being constructed or destructed. Before this fix the above then prints out (I annotated the output a bit): ``` Processing test.C... Created 0x12cb8cc68 <--- this is the static instance make ws Created 0x7ffee2baaab0 <--- the workspace's cache factory method: reading back Created 0x7fcbc7b39008 <--- the read-back workspace's cache Created 0x7fcbd45a0b70 <--- memory leaking cache deleting w2 Destroyed 0x7fcbc7b39008 deleting w Destroyed 0x7ffee2baaab0 root [1] .q Destroyed 0x12cb8cc68 ``` After the fix caches are created and destroyed as expected: ``` Processing test.C... Created 0x1290a5c68 make ws Created 0x7ffee623eab0 factory method: reading back Created 0x7f9bd8437408 deleting w2 Destroyed 0x7f9bd8437408 deleting w Destroyed 0x7ffee623eab0 root [1] .q Destroyed 0x1290a5c68 ```
-
Markus Frank authored
-
Jonas Rembser authored
The `RooAbsArg::redirectServers()` funciton has an optional flag to print an error if not all servers listed in the new server set were replaced. However, if one was already using this `mustReplaceAll` flag, it means that the error is intentional and should be considered fatal. An exception should be thrown with it.
-
Jonas Rembser authored
In the `RooSimGenContex` and `RooSimSplitGenContex` classes, there is a place where the servers of a derived category are redirected. The `mustReplaceAll` flag was set to true here. This made sense when the internal categories were still the only servers of the `RooSuperCategory`, but since the refactor in #5502 this is not the case anymore. Therefore, a harmless error is printed now, which should be avoided by setting the `mustReplaceAll` flag to `false`.
-
Jonas Rembser authored
Add unit test that checks if the dataset generation from a nested RooSimultaneous with protodata containing the category values works. Covers GitHub issue #12020.
-
Jonas Rembser authored
In the `RooSimultaneous::genContext` function, the logic that figures out which `RooAbsGenContext` implementation to instantiate needs to know the list of all category components. The way to figure this out depends on the type of the index category, and this logic is also reused in the `RooAbsGenContext` implementations for the RooSimultaneous. That's why it is now factored out into a protected function called `RooSimultaneous::flattenCatList()`.
-
Jonas Rembser authored
Plus some code modernization of the gen context implementations
-
Jonas Rembser authored
* avoid code duplication * less manual memory management * more range-based loops
-
Jonas Rembser authored
When generating such a pdf from prototype data, the prototype needs to contain all the subcategories of the super-category, and it does so by checking the super-category servers. However, recently `RooSuperCategory` was changed to contain a `RooMultiCategory` internally, and the only reported direct server is the internal multi-category. This leads to a wrong generation (the prototype data is ignored, the gen context refers to the current labels). The solution that this commit suggests is to use `RooSuperCategory::inputCatList()` to inspect the internal categories instead of using the client-server interface. Actually, the client-server interface is not meant to be used for inspection like that, just for general dependency management in the evaluation. Closes #12020.
-