Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[RF] Make RooFFTConfPdf also work if ROOT was built with fftw3=OFF #14174

Merged
merged 1 commit into from
Dec 7, 2023

Conversation

guitargeek
Copy link
Contributor

@guitargeek guitargeek commented Dec 4, 2023

Since ROOT 6.30, we are not building ROOT with the math/fftw
subpackage anymore (fftw3=OFF). It is the interface between ROOT and
fftw3, but is incompatible with fftws GPL license.

That means that for default ROOT builds, the RooFFTConvPdf class for
FFT convolutions is not working anymore, because it uses math/fftw.

This commit implements a solution to make the RooFFTConvPdf work
again in this situation: the routine that uses fftw is declared
on-the-fly to the interpreter. This will work if the user has an
external install of fftw3, which is usually available in all Linux
distributions.

Closes #14162.

Note: the error that you get if fftw3.h can't be found looks like this:

input_line_21:1:10: fatal error: 'fftw3.h' file not found
#include "fftw3.h"
         ^~~~~~~~~
[#0] ERROR:Eval -- RooFFTConvPdf evaluation Failed! The interpreter could not find the fftw3.h header.
You have three options to fix this problem:
    1) Install fftw3 on your system so that the interpreter can find it
    2) In case fftw3.h is installed somewhere else,
       tell ROOT with gInterpreter->AddIncludePath() where to find it
    3) Compile ROOT with the -Dfftw3=ON in the CMake configuration,
       such that ROOT comes with built-in fftw3 convolution routines

terminate called after throwing an instance of 'std::runtime_error'
  what():  RooFFTConvPdf evaluation Failed! The interpreter could not find the fftw3.h header

Copy link
Member

@lmoneta lmoneta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not in favour of this solution.
I don't think is bringing any advantages, we were using a plugin manager that was based on the interpreter and there were no direct dependency on fftw.
Now you require people to directly install fftw3 and they would need to set also the include path. It is an over complication for the users.

@phsft-bot
Copy link
Collaborator

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

@root-project root-project deleted a comment from phsft-bot Dec 4, 2023
@root-project root-project deleted a comment from phsft-bot Dec 4, 2023
@guitargeek
Copy link
Contributor Author

guitargeek commented Dec 4, 2023

I am not in favour of this solution.
I don't think is bringing any advantages, we were using a plugin manager that was based on the interpreter and there were no direct dependency on fftw3.
Now you require people to directly install fftw3 and they would need to set also the include path. It is an over complication for the users.

You are right, when building ROOT with fftw3=ON, it brings zero advantages but only makes if more fragile because fftw3.h needs to be in the path. Edit: it just came to my mind that this might actually be a rare case. Right now, builtin_fftw3* is always OFF by default, also on cfmfs. So the fftw3.h header was already found in the include path at build time, which means it's probably found when running ROOT too.

However, in case where ROOT is built with fftw3=OFF, this makes the difference between the RooFFTConvPdf even working or not. And this case is relevant because we release the ROOT binaries like this.

Since we need to make sure that FFTs work in RooFit when people install ROOT on their laptops with the binaries, I think a fix like this is necessary. Maybe as a compromise, I could apply the these changes only in case where fftw3=OFF by using preprocessor macros? The downside of this would be two different code branches thought.

@phsft-bot
Copy link
Collaborator

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

@phsft-bot
Copy link
Collaborator

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

@guitargeek guitargeek changed the title [RF] Don't use math/fftw but fftw directly via interpreter [RF] Make RooFFTConfPdf also work if ROOT was built with fftw3=OFF Dec 4, 2023
Copy link

github-actions bot commented Dec 4, 2023

Test Results

       10 files         10 suites   2d 3h 31m 31s ⏱️
  2 484 tests   2 483 ✔️ 0 💤 1
23 763 runs  23 762 ✔️ 0 💤 1

For more details on these failures, see this check.

Results for commit 1641f84.

♻️ This comment has been updated with latest results.

@phsft-bot
Copy link
Collaborator

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

Since ROOT 6.30, we are not building ROOT with the `math/fftw`
subpackage anymore (`fftw3=OFF`). It is the interface between ROOT and
fftw3, but is incompatible with fftws GPL license.

That means that for default ROOT builds, the `RooFFTConvPdf` class for
FFT convolutions is not working anymore, because it uses `math/fftw`.

This commit implements a solution to make the `RooFFTConvPdf` work
again in this situation: the routine that uses fftw is declared
on-the-fly to the interpreter. This will work if the user has an
external install of `fftw3`, which is usually available in all Linux
distributions.
@phsft-bot
Copy link
Collaborator

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

Copy link
Member

@lmoneta lmoneta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you, Jonas, for these changes, it is good now to have both options: use the ROOT FFTW classes (as before) or directly FFTW via the interpreter.

@guitargeek guitargeek merged commit 61c2c87 into root-project:master Dec 7, 2023
11 of 16 checks passed
@guitargeek guitargeek deleted the roofftconvpdf branch December 7, 2023 10:45
@phsft-bot
Copy link
Collaborator

Build failed on ROOT-ubuntu2004/python3.
Running on root-ubuntu-2004-3.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

  • [2023-12-07T12:26:21.736Z] CMake Error at /home/sftnight/build/workspace/root-pullrequests-build/rootspi/jenkins/root-build.cmake:1126 (message):

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RooFFTConvPdf is not working for ROOT 6.30/02
3 participants