[RF] Adding RooBSplines as a core class to RooFit #14015
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ROOT raw buffer for the shape may include more than 4 vertices in polynom. Support such case not seen before
Make `FilteringDiagConsumer` also ignore -Wunused-result. Whether or not
such diagnostic is filtered depends on `CompilationOptions::IgnorePromptDiags`.
In particular, `IgnorePromptDiags` should _only_ be enabled for code parsed
via `Interpreter::EvaluateInternal()`. Thus, as of this commit `IgnorePromptDiags`
defaults to 0 in `makeDefaultCompilationOpts()`
The observable effect of this change is ignoring `-Wunused-result` for
wrapped code, e.g.
```c++
[[nodiscard]] int f() { return 0; }
// This yields `warning: ignoring return value of function declared with 'nodiscard' attribute [-Wunused-result]`
void g() { f(); }
f(); // but this should not
```
Cherry-picked from ade784d.
This fixes root-project#12552 This commit add a mutex for the TClassTable inner operations. Several changes were needed to insure that operation that can either take the ROOT global lock or recursively call TClassTable are executed outside of the TClassTable critical sections.
The name normalization must be done outside of the TClassTable critical section, so both the normalization and lock taking needs to be done outside the FindElement function. Then rename the remaning function (FindElementImpl) into the now available FindElement. Both are private functions
The error handler will initialize gROOT if not there yet and then recursively call TClassTable
Improve documentation Use ROOT::GetROOT() rather than gROOT to trigger initialization Remove copy and default construction from RAII struct Move include where actually needed
Protect fgTally in Print Release lock before calling TProtoClass::FillTClass Add missing protection of fgIdMap in TClassTable::GetProto
It can be useful to wrap `RooAbsReal` objects that behave like self-normalized pdfs.
The existing implementation in RooBatchCompute for polynomials was not adequate, because it couldn't deal with coefficients that are different for each event. This commit is re-implementing the support for RooPolynomial, and also re-uses the same code for the `RooPolyVar`, which is identical to the `RooPolynomial` apart from the base class.
The `RunContext` in its current form was only used by the old BatchMode implementation before the RooFitDriver was introduced. Eventually, that class might be removed or merged with the `RooFit::Detail::DataMap` class that superseeded it. To make such future developments easier, the `RunContext` is now not used anymore in the `testHistFactory.cxx` file. Instead, these tests now use the `RooAbsReal::getValues()` overload that directly takes a RooDataSet. Furthermore, the validation of unnormalized pdf values in the HistFactoy unit test is removed, because unnormalized pdf values are generally undefined.
So we can eventually also use it on the GPU.
So far, the pdf normalization could not be done in RooBatchCompute, because it requires the tracking of evaluation errors. This could not be implemented in the current batchcompure library, because information could not flow back from the compute functions to the calling object. This is now made possible by taking the `ExtraArgs` array by reference, meaning it can also be used as an output parameter by the compute function to return for example the number of evaluation errors. This change greatly helps to speed up the BatchMode, expecially on the GPU because the normalization can now be done on the device.
Since this file is going to be changed a lot next, it is formatted with `clang-format` now.
Like this, we are checking that the recovery from missing values also works on the GPU. Also, do some code modernization in the test file, and fix memory leaks of RooFitResult objects.
There was a bad interaction between commit 8a67cf6 changing the signature of compute() to take a non-const reference, requiring an lvalue, and commit aa47d71 introducing a new call to that same function with a temporary argument. Fix it in the same way as other call sites were changed in commit 8a67cf6, by introducing a local variable.
In particular, this commit migrates `RooAbsPdf::fitTo()` and the functions to create cumulative density functions.
This is done now because in the RooFit code generation, we want to cache things for general RooFit collections.
The `dependsOn()` method checks the dependency only based on the name, and it is useful to make dependency checks only based on the name if one doesn't have a `RooAbsArg` with the given name at hand.
The same is done already in `stressRooStats`. Instead of manually propagating the BatchMode flag to all calls of `createNLL()` and `fitTo()`, it is easier and safer to just override the default, because then it can't be forgotten to add it in one of the calls. In particular, this is done now because it will make it much easier the extend the possible `stressRooFit` options to also use the codegen plus AD backend.
dont use normRange override, use normRange method instead Co-authored-by: Jonas Rembser <jonas.rembser@cern.ch>
This is done so the code can be re-used in the RooFuncWrapper.
Not all nodes in a RooFit computation graph support the GPU yet. That's
why in general, data needs to be copied between host and device when
evaluating the NLL.
The RooFit driver has some logic to figure out which GPU-supporting
nodes should *not* be evaluated on the GPU because the copying overhead
would negate the performance improvement.
This commit suggests to remove this logic for several reasons:
* It has no real-world benefit: copying is *much* more expensve than
evaluation, so as soon as one vector node doesn't support the GPU,
it's not worth it to do any computation on the GPU
* The NLL result is less deterministic because the splitting of compute
between GPU and CPU is based on random factors
* It makes the RooFitDriver code much harder to understand, which is
not good for new contributors (like the summer student we will soon
have)
* Determining how compute should be assigned has performance overhead
Some refactoring to make the `RooFitDriver` code more robust.
…oject#13472) * [windows] Make `OpenDirectory` and `GetDirEntry` thread safe Create a struct holding the flag, `HANDLE`, and `WIN32_FIND_DATA` used by `OpenDirectory`, `GetDirEntry`, and `FreeDirectory`, so each thread creates its own instance of it. This fixes randome failures in mutithreaded applications on Windows, like for example in the `R__USE_IMT` part of the `datasource-root` test: ``` [ RUN ] TRootTDS.DefineSlotMT Error in <TFile::TFile>: file C:/root-dev/build/x64/debug/tree/dataframe/test/G__NTupleStruct.vcx does not exist [ OK ] TRootTDS.DefineSlotMT (191 ms) [ RUN ] TRootTDS.FromARDFMT Error in <TFile::Init>: C:/root-dev/build/x64/debug/tree/dataframe/test/INSTALL.vcxproj not a ROOT file C:\root-dev\git\master\tree\dataframe\test\datasource_root.cxx(175): error: Expected equality of these values: 29. Which is: 29 *max Which is: 23 [ FAILED ] TRootTDS.FromARDFMT (6 ms) ``` * Adress the comments from Axel * Pass the correct pointer to the helper * Cosmetics * Update core/winnt/src/TWinNTSystem.cxx Prevent possible memory leak Co-authored-by: Philippe Canal <pcanal@fnal.gov> * Update core/winnt/src/TWinNTSystem.cxx --------- Co-authored-by: Philippe Canal <pcanal@fnal.gov>
It only appears in GCC 7, but C++14 is already supported since GCC 6 and users want to build ROOT 6.28 using that compiler.
vincecr0ft
requested review from
bellenot,
linev,
osschar,
couet,
eguiraud,
jblomer,
vepadulano,
pcanal,
lmoneta,
sitongan,
Axel-Naumann,
guitargeek,
hageboeck,
gganis and
agheata
as code owners
|
Can one of the admins verify this patch? |
|
Please rebase your branch against master. |
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The RooBSplines code was originally developed by Aaron Armbruster and maintained internally within the ATLAS collaboration since 2012. This PR adds the
RooBSplinesclass and theRooBSplineBasesclass to the core RooFit ecosystem.Changes or fixes:
added
RooBSplines.cxxandRooBSplineBases.cxxwith associated headers and CMakeLists and LinkDef updates.package was moved to /root/roofit/roofit/ rather than roostats/histfactory
Checklist: