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

Consider switching to easyfft #28

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

WalterSmuts
Copy link

This makes the logic simpler and manages the planner and scratch buffers behind the scenes resulting in a ~80% benchmark improvement across the board:

McLeod get_pitch        time:   [11.114 µs 11.118 µs 11.121 µs]
                        change: [-81.978% -81.964% -81.951%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 2 outliers among 100 measurements (2.00%)
  1 (1.00%) low severe
  1 (1.00%) high mild

Autocorrelation get_pitch
                        time:   [7.9310 µs 7.9348 µs 7.9388 µs]
                        change: [-86.719% -86.711% -86.703%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 3 outliers among 100 measurements (3.00%)
  2 (2.00%) low mild
  1 (1.00%) high mild

YIN get_pitch           time:   [8.8091 µs 8.8150 µs 8.8222 µs]
                        change: [-78.127% -78.106% -78.085%] (p = 0.00 < 0.05)
                        Performance has improved.
Found 13 outliers among 100 measurements (13.00%)
  3 (3.00%) high mild
  10 (10.00%) high severe

detect_peaks            time:   [523.75 ns 524.12 ns 524.48 ns]
                        change: [+0.0519% +0.3183% +0.5176%] (p = 0.00 < 0.05)
                        Change within noise threshold.
Found 5 outliers among 100 measurements (5.00%)
  4 (4.00%) high mild
  1 (1.00%) high severe

Unfortunately it requires an extra trait bound on Default for the fft elements. This is not neccesary but requires changes to rustfft tracked in ejmahler/RustFFT#105.

@alesgenova
Copy link
Owner

Good morning,

Thank you for the contribution!

I don't see any problem with this PR and I will get it merged, just want to make sure that I can still build the library for wasm first.

You seem to know your way around the various fft libraries, so I have an unrelated question for you.

I was trying to get this library built for a no_std environment, but I didn't get far as rustfft brings in a few dependencies (mainly the num traits) that are no_std compatible but only when specifying some flags, which rustfft doesn't expose to its consumers.

Do you know if there is a workaround for this?

@WalterSmuts
Copy link
Author

I don't have much experience with no_std but here goes some info. Take it with a grain of salt however because I've not ever really been involved:

  • rustfft uses std a LOT:
$ git grep std | wc
    347    1250   25387
  • Some of rustfft's functionality relies on Allocation such as the convenient general planner:
let mut planner = FftPlanner::new();
let fft = planner.plan_fft_forward(1234); // <-- This returns an Arc<dyn Fft<T>>

so you'd atleast need an allocator which AFAIK can be achieved without std.

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

Successfully merging this pull request may close these issues.

2 participants