FFT module revamp + updating dune-lang #680

gabyfle · 2024-11-10T21:21:20Z

Introduction

Hello everyone. In order to be able to add more features to the Audio processing library I'm currently building on top of Owl (see: https://github.com/gabyfle/SoundML), I needed to be able to compute Discrete Cosine & Sine transforms (in order to compute MFCC and other related things). The Owl's FFT module was so far pretty empty even if very usefull, only exposing real and complex discrete fourier transforms, but with only few prameters available.

After checking out what was done in the well known Scipy Python library and comparing with what was done inside Owl, I found out that Owl was using FFTPACK for the computation. In order to stay a maximum compatible with the existing Owl's API, I used (like in the Scipy library) pocketfft which comes as a C++ header-only library.

In fact these additions should not break any code (it's 100% same API), and it might even enhance it because we gained some of the features of pocketfft:

It is now possible to specify the number of threads used to make the computation (meaning that for large arrays, we might gain from the multi-threaded code of pocketfft)
4 new functions were added : dct, idct, dst, idst inside the Owl.Fft module
Deleted old FFTPACK code
Updated the ocamlformat version to the current latest (0.26.2) and deleted deprecated options from the .ocamlformat file
Updated the dune version to the current latest (3.16) and added flags to remove warnings inside the examples folder

I hope that these new features / enhancements will please you ! Please, feel free to let me know if you find out anything wrong/unexpected.

gabyfle.

Main changes:

Fast Fourier Transform API:

val fft:  ?axis:int -> ?norm:int -> ?nthreads:int -> (Complex.t, 'a) t -> (Complex.t, 'a) t
  
val ifft:  ?axis:int -> ?norm:int -> ?nthreads:int -> (Complex.t, 'a) t -> (Complex.t, 'a) t
  
val rfft :  ?axis:int -> ?norm:int -> ?nthreads:int -> otyp:('a, 'b) kind -> ('c, 'd) t -> ('a, 'b) t
  
val irfft :  ?axis:int -> ?n:int -> ?norm:int -> ?nthreads:int -> otyp:('a, 'b) kind -> ('c, 'd) t -> ('a, 'b) t
  
val dct :  ?axis:int -> ?ttype:int -> ?norm:int -> ?ortho:bool -> ?nthreads:int -> ('a, 'b) t -> ('a, 'b) t
  
val idct :  ?axis:int -> ?ttype:int -> ?norm:int -> ?ortho:bool -> ?nthreads:int -> ('a, 'b) t -> ('a, 'b) t
  
val dst :  ?axis:int -> ?ttype:int -> ?norm:int -> ?ortho:bool -> ?nthreads:int -> ('a, 'b) t -> ('a, 'b) t
  
val idst :  ?axis:int -> ?ttype:int -> ?norm:int -> ?ortho:bool -> ?nthreads:int -> ('a, 'b) t -> ('a, 'b) t

Dune version from (lang dune 2.0) to (lang dune 3.16)
ocamlformat version from version=0.20.0 to version=0.26.2

hhugo · 2024-11-10T22:01:20Z

Things would be much easier to review if ocamlformat update and dune lang bump were in two separate commits and not mixed with owl logic.

gabyfle · 2024-11-10T22:10:12Z

Things would be much easier to review if ocamlformat update and dune lang bump were in two separate commits and not mixed with owl logic.

Yes actually that's what I was thinking about when seeing the enormous diff. I'll revert the changes made by the new version of OCaml format. And push a new commit so it'll be easier for you to check the changes.

- Added several new functionnalities to the FFT module by changing the dependency from FFTPACK to POCKETFTT. - new optionnal parameters for the API (nthreads, norm, ...) - new functions for cosine and sine transforms (dct, dst, ...) - Switched from dune 2.0 to dune 3.16 (this was required as I ran throught issues with linking while using 2.0)

gabyfle · 2024-11-11T00:54:22Z

I force-pushed to the branch only the modifications I added to the Owl logic, and discarded the ones that were due to the ocamlformat version update. The diff should be way smaller now for code review.

mseri · 2024-11-11T10:28:03Z

src/owl/fftpack/owl_fft_generic.mli

+
+val dct
+  :  ?axis:int
+  -> ?ttype:int


What does this stand for? You could avoid the asserts like assert (ttype > 0 || ttype < 5) by introducing a new more descriptive type for these that converts to the specific int and enforces the constraints. However, I am not sure if this impacts performances (I think it can be erased by the compiler)

Hi, thanks for your review. The ttype option is for the Cosine and Sine transforms type (see scipy.fft.dct and scipy.fft.dst for references as I inspired the implementation from their).

Regarding the assert, I was trying to stick with the current state of Owl. Lots of conditions checking are done with asserts through the code (which I agree with you is not optimal). Sure thing, introducing a custom type here might be beneficial.

Thanks !

- Added the ttrig_transform type to specify the DCT and DST types - Added the tnorm type to specify the normalization option for the FFTs.

jzstark · 2024-11-11T17:12:52Z

@gabyfle Thanks a lot for this nice contribution! Do you think you could also add some extra examples and tests for the revised FFT module?

gabyfle · 2024-11-11T17:43:15Z

@gabyfle Thanks a lot for this nice contribution! Do you think you could also add some extra examples and tests for the revised FFT module?

Hi, thanks for the review. I just saw that CI failed, I'll try to investigate as it compiles well on my Ubuntu installation.

Of course, I'll add some tests though I'm unfamiliar with Alcotest, I'll try my best and feel free to correct me if I'm misusing it.

EDIT: looks like the main issue is the pocketfft not being cloned inside the repo while the workflow is running, causing the error pocketfft_hdronly.h: no such file or directory. I'm unsure of how to fix that, and it's also beyond the scope of this PR so if someone from the repo as any idea to fix this, I
it'd be welcome !

- One "complex" usage: computing a PSD spectrogram from input data - One "simple" usage: computing the maximum frequency peak in time series data using rfft.

- Fixed an issue where the DCT/DST parameters weren't passed in the right order - Fixed an issue where the norm factor of the DCT wasn't computed correctly (incorrect delta) - Generated a unit_fft file that tests various parameters of FFT. Values are generated using scipy.fft module for the FFT, DCT and DST functions - Changed pocketfft::detail:: namespace usage to just pocketfft:: in the C++ code for more readability

gabyfle · 2024-11-12T01:49:28Z

@jzstark I added a tests/unit_fft.ml file that tests the (64bits) module using various data generated from values computed with scipy.fft module for reference.

This allowed me to spot two little issues in the implementation:

I was passing arguments to the C code in wrong order for the DCT and DST functions (which I didn't spot since I was using axis:1 in my testings!)
The normalization factor of the DCT function was incorrect due to a wrong delta (which in fact is a direct consequence from copy/pasting code from the DST function)

These issues are fixed and all the added tests are passing. I'm unsure whether or not I'm using Alcotest correctly, I did tried to stick to the same scheme used by Owl for the testing of the other modules. Let me know if there's any issue regarding the tests added, or if you're willing to add any test.

EDIT: I'm already using these modifications inside my sound library code. You can see DCT in action here: https://github.com/gabyfle/SoundML/blob/feature/mfcc/src/feature/spectral/spectral.ml#L249-L285

gabyfle · 2024-11-17T18:58:59Z

I'll get rid of the submodule and directly put a copy of the pocketfft library into the repository to avoid CI failing.

gabyfle · 2024-11-17T23:13:27Z

Hi @jzstark,

The CI should be fixed. I added manually the pocketfft code into the repository, so the pocketfft_hdronly.h: no such file or directory should not occur anymore. Let me know what do you think of this !

jzstark · 2024-11-18T10:57:51Z

Thank you so much for the fix @gabyfle ! Really appreciate the workload doing this. It already works on the ubuntu + OCaml 5.1 environment, which is actually quite nice. But it would still be great if we can find out how to fix the issues on mac and OCaml 4.x versions. Unfortunately I don't have the environments at hands to give it a quick local debugging now.

gabyfle · 2024-11-18T12:02:00Z

@jzstark Ubuntu compile for versions 4.x should be fixed now. The only issue I'll have now is that I don't have a MacOS computer to run the builds on my machine for the moment. I'll let you know once it's done !

- Kept the declaration inside the extern C but moved away the declaration, as an attempt to fix MacOS builds.

mseri · 2024-11-18T15:11:58Z

This works well and passes all the tests with ocaml 4.14.1 on my mac.

I don't understand the failure for 4.10, see below.
However I think we could just drop support: do we really care about supporting 4.10? I think anything after 4.12 or even 4.14 would be more than enough.

(cd _build/default/src/owl && /usr/bin/cc -x c++ -O2 -fno-strict-aliasing -fwrapv -fdiagnostics-color=always -std=c++11 -g -O3 -march=native -funroll-loops -fno-math-errno -fno-rounding-math -fno-signaling-nans -fexcess-precision=fast -DSFMT_MEXP=19937 -fno-strict-aliasing -I/opt/homebrew/Cellar/openblas/0.3.28/include -g -I /Users/runner/work/owl/owl/_opam/lib/ocaml -I /Users/runner/work/owl/owl/_opam/lib/bigarray-compat -I /Users/runner/work/owl/owl/_opam/lib/camlzip -I /Users/runner/work/owl/owl/_opam/lib/ctypes -I /Users/runner/work/owl/owl/_opam/lib/ctypes/stubs -I /Users/runner/work/owl/owl/_opam/lib/integers -I /Users/runner/work/owl/owl/_opam/lib/npy -I /Users/runner/work/owl/owl/_opam/lib/owl-base -I /Users/runner/work/owl/owl/_opam/lib/stdlib-shims -I /Users/runner/work/owl/owl/_opam/lib/zip -o owl_fftpack_float64.o -c owl_fftpack_float64.cc)
clang: warning: optimization flag '-fno-signaling-nans' is not supported [-Wignored-optimization-argument]
clang: warning: optimization flag '-fexcess-precision=fast' is not supported [-Wignored-optimization-argument]
In file included from src/owl/fftpack/owl_fftpack_float64.cc:7:
In file included from src/owl/core/owl_core.h:9:
In file included from src/owl/core/owl_macros.h:9:
In file included from /Applications/Xcode_15.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/v1/math.h:315:
In file included from /Applications/Xcode_15.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/v1/limits:117:
/Users/runner/work/owl/owl/_opam/lib/ocaml/version:1:1: error: expected unqualified-id
4.10.2
^
/Users/runner/work/owl/owl/_opam/lib/ocaml/version:3:3: error: invalid preprocessing directive
# The version string is the first line of this file.
  ^
/Users/runner/work/owl/owl/_opam/lib/ocaml/version:4:3: error: invalid preprocessing directive
# It must be in the format described in stdlib/sys.mli
  ^
In file included from src/owl/fftpack/owl_fftpack_float64.cc:7:
In file included from src/owl/core/owl_core.h:9:
In file included from src/owl/core/owl_macros.h:9:
In file included from /Applications/Xcode_15.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/v1/math.h:315:
In file included from /Applications/Xcode_15.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/v1/limits:827:
In file included from /Applications/Xcode_15.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/v1/type_traits:425:
In file included from /Applications/Xcode_15.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/v1/__type_traits/add_pointer.h:17:
In file included from /Applications/Xcode_15.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/v1/__type_traits/remove_reference.h:13:
In file included from /Applications/Xcode_15.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/v1/cstddef:41:
/Users/runner/work/owl/owl/_opam/lib/ocaml/version:1:1: error: expected unqualified-id
4.10.2
^
/Users/runner/work/owl/owl/_opam/lib/ocaml/version:3:3: error: invalid preprocessing directive
# The version string is the first line of this file.
  ^
/Users/runner/work/owl/owl/_opam/lib/ocaml/version:4:3: error: invalid preprocessing directive
# It must be in the format described in stdlib/sys.mli
  ^

The failure in https://github.com/owlbarn/owl/actions/runs/11894830732/workflow is on homebrew side: macos workers come with an old version of base packages preinstalled and now conflict with the new pkgconfig package (which changed name to pkgconf). It has nothing to do with this PR, I'll send a fix attempt separately. The failure of https://github.com/owlbarn/owl/actions/runs/11894830682?pr=680 is due to some issue with github pages actions, independent of this PR

jzstark · 2024-11-18T15:41:46Z

@mseri Thanks a lot for looking into this issue. Yes I think the error on 4.10 is more likely an Action problem with the OCaml version numbers now that the installation works on other platforms. I will first update the github action setup with your new PR.

@gabyfle Thanks for the quick fixes and very solid PR!

gabyfle · 2024-11-18T16:21:55Z

This works well and passes all the tests with ocaml 4.14.1 on my mac.

I don't understand the failure for 4.10, see below. However I think we could just drop support: do we really care about supporting 4.10? I think anything after 4.12 or even 4.14 would be more than enough.

(cd _build/default/src/owl && /usr/bin/cc -x c++ -O2 -fno-strict-aliasing -fwrapv -fdiagnostics-color=always -std=c++11 -g -O3 -march=native -funroll-loops -fno-math-errno -fno-rounding-math -fno-signaling-nans -fexcess-precision=fast -DSFMT_MEXP=19937 -fno-strict-aliasing -I/opt/homebrew/Cellar/openblas/0.3.28/include -g -I /Users/runner/work/owl/owl/_opam/lib/ocaml -I /Users/runner/work/owl/owl/_opam/lib/bigarray-compat -I /Users/runner/work/owl/owl/_opam/lib/camlzip -I /Users/runner/work/owl/owl/_opam/lib/ctypes -I /Users/runner/work/owl/owl/_opam/lib/ctypes/stubs -I /Users/runner/work/owl/owl/_opam/lib/integers -I /Users/runner/work/owl/owl/_opam/lib/npy -I /Users/runner/work/owl/owl/_opam/lib/owl-base -I /Users/runner/work/owl/owl/_opam/lib/stdlib-shims -I /Users/runner/work/owl/owl/_opam/lib/zip -o owl_fftpack_float64.o -c owl_fftpack_float64.cc)
clang: warning: optimization flag '-fno-signaling-nans' is not supported [-Wignored-optimization-argument]
clang: warning: optimization flag '-fexcess-precision=fast' is not supported [-Wignored-optimization-argument]
In file included from src/owl/fftpack/owl_fftpack_float64.cc:7:
In file included from src/owl/core/owl_core.h:9:
In file included from src/owl/core/owl_macros.h:9:
In file included from /Applications/Xcode_15.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/v1/math.h:315:
In file included from /Applications/Xcode_15.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/v1/limits:117:
/Users/runner/work/owl/owl/_opam/lib/ocaml/version:1:1: error: expected unqualified-id
4.10.2
^
/Users/runner/work/owl/owl/_opam/lib/ocaml/version:3:3: error: invalid preprocessing directive
# The version string is the first line of this file.
  ^
/Users/runner/work/owl/owl/_opam/lib/ocaml/version:4:3: error: invalid preprocessing directive
# It must be in the format described in stdlib/sys.mli
  ^
In file included from src/owl/fftpack/owl_fftpack_float64.cc:7:
In file included from src/owl/core/owl_core.h:9:
In file included from src/owl/core/owl_macros.h:9:
In file included from /Applications/Xcode_15.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/v1/math.h:315:
In file included from /Applications/Xcode_15.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/v1/limits:827:
In file included from /Applications/Xcode_15.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/v1/type_traits:425:
In file included from /Applications/Xcode_15.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/v1/__type_traits/add_pointer.h:17:
In file included from /Applications/Xcode_15.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/v1/__type_traits/remove_reference.h:13:
In file included from /Applications/Xcode_15.4.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/c++/v1/cstddef:41:
/Users/runner/work/owl/owl/_opam/lib/ocaml/version:1:1: error: expected unqualified-id
4.10.2
^
/Users/runner/work/owl/owl/_opam/lib/ocaml/version:3:3: error: invalid preprocessing directive
# The version string is the first line of this file.
  ^
/Users/runner/work/owl/owl/_opam/lib/ocaml/version:4:3: error: invalid preprocessing directive
# It must be in the format described in stdlib/sys.mli
  ^

The failure in https://github.com/owlbarn/owl/actions/runs/11894830732/workflow is on homebrew side: macos workers come with an old version of base packages preinstalled and now conflict with the new pkgconfig package (which changed name to pkgconf). It has nothing to do with this PR, I'll send a fix attempt separately. The failure of https://github.com/owlbarn/owl/actions/runs/11894830682?pr=680 is due to some issue with github pages actions, independent of this PR

Thanks a lot for looking after this issue, I was tearing my hair out over this problem, not understanding what did I do wrong ! Glad to see it's not on my side !

@mseri Thanks a lot for looking into this issue. Yes I think the error on 4.10 is more likely an Action problem with the OCaml version numbers now that the installation works on other platforms. I will first update the github action setup with your new PR.

@gabyfle Thanks for the quick fixes and very solid PR!

No problem, thanks to you to keep updated Owl and maintening the project so far !

- Added norm and nthreads parameters.

jzstark · 2024-11-19T18:32:51Z

@gabyfle Now the checks work fine (except for the 4.10 version we have already decide to dump). So everything looks fine other than the SciPy performance gap problem you mentioned. Please let me know if you think the current results are better and this PR is ready to go. (Perhaps also incorporate this performance gap check into the unit test?)

gabyfle · 2024-11-19T18:47:51Z

@jzstark Thanks for your reply.

Actually, there are no performances gap (that's why I deleted the comment). I was checking results computed with Float32 precision against float64 from scipy, leading to the confusion of performances gaps.

I personnally tested it on few testing audio I have for the library I'm building, and it works like a charm.

jzstark · 2024-11-19T19:03:33Z

Perfect! Thanks for this great PR again!

gabyfle force-pushed the main branch from 0f6195c to 98a73c4 Compare November 11, 2024 00:50

gabyfle changed the title ~~Tools update and FFT revamp !~~ FFT module revamp + updating dune-lang Nov 11, 2024

mseri reviewed Nov 11, 2024

View reviewed changes

Adding abstract types and documentation.

6d324e9

- Added the ttrig_transform type to specify the DCT and DST types - Added the tnorm type to specify the normalization option for the FFTs.

gabyfle requested a review from mseri November 11, 2024 13:22

gabyfle added 2 commits November 11, 2024 20:52

Adding 2 FFT example usage.

aea4703

- One "complex" usage: computing a PSD spectrogram from input data - One "simple" usage: computing the maximum frequency peak in time series data using rfft.

gabyfle added 3 commits November 18, 2024 00:08

Remove pocketfft submodule from .gitmodules

9fd7bb0

Convert pocketfft submodule to regular directory

c05f7dc

Merge branch 'owlbarn:main' into main

5e45f71

Ubuntu 4.x build fix. MacOS fix attempt.

41752c5

Moved the definition outside extern C.

e9a1f08

- Kept the declaration inside the extern C but moved away the declaration, as an attempt to fix MacOS builds.

gabyfle force-pushed the main branch from e125c20 to e9a1f08 Compare November 18, 2024 14:04

owl_core preprocessor for cpp compiling.

5fcd0c3

gabyfle added 2 commits November 19, 2024 09:45

Adapted the fft2 and ifft2 prototypes to match new FFT module.

85e5ae9

- Added norm and nthreads parameters.

Fixing documentation strings.

2ab8c8d

gabyfle marked this pull request as draft November 19, 2024 11:47

Little code change for more similarities with scipy's implementation.

8fa2d85

gabyfle marked this pull request as ready for review November 19, 2024 17:51

jzstark merged commit 8cec08f into owlbarn:main Nov 19, 2024
12 of 14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FFT module revamp + updating dune-lang #680

FFT module revamp + updating dune-lang #680

gabyfle commented Nov 10, 2024 •

edited

Loading

hhugo commented Nov 10, 2024

gabyfle commented Nov 10, 2024

gabyfle commented Nov 11, 2024

mseri Nov 11, 2024 •

edited

Loading

gabyfle Nov 11, 2024

jzstark commented Nov 11, 2024

gabyfle commented Nov 11, 2024 •

edited

Loading

gabyfle commented Nov 12, 2024 •

edited

Loading

gabyfle commented Nov 17, 2024

gabyfle commented Nov 17, 2024

jzstark commented Nov 18, 2024

gabyfle commented Nov 18, 2024

mseri commented Nov 18, 2024

jzstark commented Nov 18, 2024

gabyfle commented Nov 18, 2024

jzstark commented Nov 19, 2024

gabyfle commented Nov 19, 2024

jzstark commented Nov 19, 2024

FFT module revamp + updating dune-lang #680

FFT module revamp + updating dune-lang #680

Conversation

gabyfle commented Nov 10, 2024 • edited Loading

Introduction

Main changes:

hhugo commented Nov 10, 2024

gabyfle commented Nov 10, 2024

gabyfle commented Nov 11, 2024

mseri Nov 11, 2024 • edited Loading

Choose a reason for hiding this comment

gabyfle Nov 11, 2024

Choose a reason for hiding this comment

jzstark commented Nov 11, 2024

gabyfle commented Nov 11, 2024 • edited Loading

gabyfle commented Nov 12, 2024 • edited Loading

gabyfle commented Nov 17, 2024

gabyfle commented Nov 17, 2024

jzstark commented Nov 18, 2024

gabyfle commented Nov 18, 2024

mseri commented Nov 18, 2024

jzstark commented Nov 18, 2024

gabyfle commented Nov 18, 2024

jzstark commented Nov 19, 2024

gabyfle commented Nov 19, 2024

jzstark commented Nov 19, 2024

gabyfle commented Nov 10, 2024 •

edited

Loading

mseri Nov 11, 2024 •

edited

Loading

gabyfle commented Nov 11, 2024 •

edited

Loading

gabyfle commented Nov 12, 2024 •

edited

Loading