Add functions to support interleaved complex num #27
Conversation
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #27 +/- ##
=======================================
Coverage 99.16% 99.16%
=======================================
Files 8 8
Lines 841 841
=======================================
Hits 834 834
Misses 7 7 ☔ View full report in Codecov by Sentry. |
|
Hi @calebzulawski, We now have the basics of what we'd need to support signals that are stored as an array of complex nums. Looking forward to hearing your thoughts! Best, |
Cargo.toml
Outdated
| @@ -13,6 +13,7 @@ exclude = ["assets", "scripts", "benches"] | |||
| [dependencies] | |||
| num-traits = "0.2.18" | |||
| multiversion = "0.7" | |||
| num-complex = "0.4.5" | |||
There was a problem hiding this comment.
I think this should be an optional feature, maybe even a non-default one but enabled on docs.rs. The associated functions then need to be annotated as available with this feature only; there is a rustdoc feature for that.
There was a problem hiding this comment.
I made the following changes:
num-complexis now a optional and non-default feature- associated functions and tests are annotated as available with this feature only
- doc will not creates docs for all features (not sure if that's the desired outcome here?)
- update the CI pipeline to run tests for all features (since I had to annotate tests that use
Complex)
|
Something like this looks good to me! Are you open to possibly using a more trait-oriented design in the future? I wouldn't want to hold up this functionality, but I think various input and output types could be combined into one generic function. |
|
Hi @calebzulawski, Thank you for the review! I think your trait-oriented solution would be great. Just to clarify, do we want to wait on merging this PR? Or do we want this PR as an interim solution? Best, |
|
I would probably merge it to have something to start from--just something to consider before a 1.0 release |
|
I've taken the liberty of rewriting I couldn't get it to vectorize on godbolt, but at least it should have the right public API, and we can always optimize it later. |
|
I tried a bunch more things and couldn't get it to autovectorize: https://godbolt.org/z/7Kd8v4vPG We might need something like bytemuck here to view a |
|
I think the approach I would take is iterate by chunks, bytemuck to |
|
Yeah that sounds good to me |
|
I am working on a simd-complex crate--this seems like an important enough algorithm to include there as an AoSoA <-> AoS conversion |
- Make num-complex optional and non-default, and all functions that take and test interleaved FFT are now under the same feature - Bump num-complex version 0.4.5 --> 0.4.6 - Enable num-complex for docs/docs.rs - Update github action workflow to run tests for all features - Fix formatting in docs to fix links
|
I have a rough version of Let me know your thoughts. Thank you! |
|
I've taken a look at the Godbolt version. Wow, that is some aggressive loop unrolling! It looks good to me though. One more thing to try: we need to benchmark if iterating over |
Use bytemuck + SIMD::deinterleave to rearrange input data from a slice of Complex values into 2 slices of f32 or f64 values
I added a benchmark, using criterion, to test this idea. The benchmark is probably not leveraging criterion to its full potential, so let me know. Edit: Added in results |
@calebzulawski @Shnatsel
A draft PR to address issue #22.