Repository navigation
[RF] Drop the FFTW3 dependency of RooFFTConvPdf - #24012
Conversation
Test Results 22 files 22 suites 3d 10h 0m 52s ⏱️ Results for commit 7bccc45b. ♻️ This comment has been updated with latest results. |
vgvassilev
left a comment
vgvassilev
left a comment
There was a problem hiding this comment.
Lgtm! Ideally clad should be able to go through the library itself. Do we know where clad fails?
You mean the |
We can probably put clad as part of rootcling and the gradients inside the dictionary or having a pullbacks generated as code (which is the same). |
Okay, maybe that's possible but I was honestly quite happy to also get rid of a library dependency of RooFit at the same time 🙂 RooFit used FFTW3 only in one single place, and in such a way that it also incurred overhead from filling FFTW3 input buffers. So the FFTW3-free implementation is better not only for AD reasons actually. |
Replace the external FFT backend of RooFFTConvPdf with a self-contained
implementation, so that the class works in any ROOT build regardless of
whether fftw3 is available.
Until now, RooFFTConvPdf used the TVirtualFFT plugin when ROOT was
compiled with `fftw3=ON`, and otherwise fell back to declaring a wrapper
around fftw3.h through the interpreter at runtime. The latter throws if
the header cannot be found, so the PDF was effectively unusable in builds
without fftw3, and the double code path had to be kept in sync.
The new implementation is built on a single iterative radix-2
Cooley-Tukey FFT with a precomputed twiddle-factor table, which keeps the
round-off error at the epsilon level independently of the transform size.
Since the sampling size N2 is not guaranteed to be a power of two, the
circular convolution for arbitrary N2 is computed exactly by zero-padding
to the next power of two >= 2*N2 - 1, which yields the linear
convolution, and folding the result back modulo N2. Both real input
sequences are packed into a single complex sequence and their spectra are
recovered via Hermitian symmetry, so one convolution costs only two
transforms. The transform is unnormalized, matching the FFTW convention;
the absolute scale is irrelevant anyway since RooHistPdf renormalizes the
cached histogram. Since the convolution of two non-negative inputs is
mathematically non-negative, round-off negatives in bins whose true value
is ~0 are clamped to zero, avoiding spurious "p.d.f value is less than
zero" evaluation errors downstream.
The result agrees with the previous FFTW-based implementation at the
level of machine precision (validated against a long-double reference
convolution), and end-to-end cache refills are about a factor 3 slower
than with FFTW, which is largely masked by the cost of sampling the input
pdfs.
As a consequence:
* RooFitCore no longer links against FFTW::Double and the
ROOFIT_MATH_FFTW3 compile definition is gone.
* The three TVirtualFFT plans are removed from FFTCacheElem.
* The documentation on how to install FFTW manually is dropped.
* stressRooFit test 208 no longer needs to check for the fftw3 build
feature and the TVirtualFFT plugin, and now always runs (except with
the codegen backend).
🤖 Done with the help of AI
98c4b28 to
7bccc45
Compare
|
Have to defer the AD support to a separate PR, because I coupled it with a bugfix that also affects the HS3 test suite. Just merging the first commit for now. |
guitargeek
changed the title
Replace the external FFT backend of RooFFTConvPdf with a self-contained implementation, so that the class works in any ROOT build regardless of whether fftw3 is available.
Until now, RooFFTConvPdf used the TVirtualFFT plugin when ROOT was compiled with
fftw3=ON, and otherwise fell back to declaring a wrapper around fftw3.h through the interpreter at runtime. The latter throws if the header cannot be found, so the PDF was effectively unusable in builds without fftw3, and the double code path had to be kept in sync.The new implementation is built on a single iterative radix-2 Cooley-Tukey FFT with a precomputed twiddle-factor table, which keeps the round-off error at the epsilon level independently of the transform size. Since the sampling size N2 is not guaranteed to be a power of two, the circular convolution for arbitrary N2 is computed exactly by zero-padding to the next power of two >= 2*N2 - 1, which yields the linear convolution, and folding the result back modulo N2. Both real input sequences are packed into a single complex sequence and their spectra are recovered via Hermitian symmetry, so one convolution costs only two transforms. The transform is unnormalized, matching the FFTW convention; the absolute scale is irrelevant anyway since RooHistPdf renormalizes the cached histogram. Since the convolution of two non-negative inputs is mathematically non-negative, round-off negatives in bins whose true value is ~0 are clamped to zero, avoiding spurious "p.d.f value is less than zero" evaluation errors downstream.
The result agrees with the previous FFTW-based implementation at the level of machine precision (validated against a long-double reference convolution), and end-to-end cache refills are about a factor 3 slower than with FFTW, which is largely masked by the cost of sampling the input pdfs.
As a consequence:
🤖 Done with the help of AI