use FloatConst/Float and from_usize to reduce conversion loss #63
Conversation
| use std::fmt::Debug; | ||
|
|
||
| /// Generic floating point number, implemented for f32 and f64 | ||
| pub trait FftNum: Copy + FromPrimitive + Signed + Sync + Send + Debug + 'static {} | ||
| pub trait FftNum: Copy + FromPrimitive + Signed + Sync + Send + Debug + Float + FloatConst + 'static {} |
There was a problem hiding this comment.
As far as I'm aware this is a breaking change, so unfortunately we will not be able to add these trait bounds.
There was a problem hiding this comment.
Yes this will cause trouble for anyone who has implemented FftNum for a type that doesn't implement the Float trait (which contains quite a lot of stuff).
I came to think about this one: #7
| @@ -161,7 +161,7 @@ impl<A: AvxNum, T: FftNum> RadersAvx2<A, T> { | |||
| } as usize; | |||
|
|
|||
| // precompute the coefficients to use inside the process method | |||
| let unity_scale = T::from_f64(1f64 / inner_fft_len as f64).unwrap(); | |||
| let unity_scale = T::from_usize(inner_fft_len).unwrap().recip(); | |||
| @@ -50,7 +50,7 @@ pub struct BluesteinsAlgorithm<T> { | |||
|
|
|||
| impl<T: FftNum> BluesteinsAlgorithm<T> { | |||
| fn compute_bluesteins_twiddle(index: usize, len: usize, direction: FftDirection) -> Complex<T> { | |||
| let index_float = index as f64; | |||
| let index_float = T::from_usize(index).unwrap(); | |||
|
I would propose to use 2*pi instead of Tau. Tau isn't a very well known constant so it can be a little confusing. For me, and probably a lot of other people, it's a time constant. |
|
Actually - as I think about this more, I would like to approach this in a different way. I'd like f32 to continue using f64 for butterflies, because it improves the precision of f32. The changes in this PR would reduce the precision for f32, which I want to avoid unless we have no other choice. I can easily think of a solution that involves specialization - @roosephu do you run on nightly or on stable? If you were willing to run on nightly, we could devise a solution where RustFFT compiles on stable, and then you provide a specialization that computes the twiddles with higher precision. |
|
I can use the nightly version, as it's a personal projection so I can whatever feature I want. I do believe specialization is the future but it seems there's a long haul for it to be stabilized, given its current status -- but I'm not opposing specialization. |
|
ok! in that case, what I'm thinking of doing is making the twiddle factor function a provided method on the FftNum trait. That would compile fine on stable, and you could specialize it to use a more-accurate computation on your own type. The compiler might give you trouble about orphan rules, but you can get around that with a newtype. As for floatindex, what I might do instead is "bigindex", where the index is a u128 instead of a u64 or f64. The only reason floatindex exists is because bluestein's algorithm uses i^2 instead of just i for its twiddle factor indexes, so we need something that can store numbers larger than u64 max. u128 will do fine for that. |
|
I'm sorry to say that after looking into this, I don't think RustFFT can support this until Rust supports specialization on stable. I implemented the two fixes to precision problems in rader's algorithm and bluestein's algorithm, but we're fundamentally stuck with f64 precision for twiddles. We can't add trait bounds to FftNum, and specialization will require specialization-aware changes to the definition of FftNum, neither of which are on the table. I'm closing this for now, but if you have any bursts of inspiration on how to solve this problem, please bring it up. |
|
So now we calculate all twiddles in f64 and then convert to T. |
This is a follow-up to #62.
I'm not familiar with the codebase so I only tried to find
as f64orT::from_f64and modify them accordingly. It's very possible that I missed something.I also ran
cargo testand it seems to be fine.