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

Add generic implementation for FFTnum #7

Closed
wants to merge 5 commits into from
Closed

Add generic implementation for FFTnum #7

wants to merge 5 commits into from

Conversation

prehner
Copy link

@prehner prehner commented Nov 30, 2020

This PR changes the supertraits of FFTnum from Signed to Num + Neg<Output = Self>.
Signed is overly restrictive and not used within the crate beyond Neg.

The generic impl:

impl<T> FFTnum for T where T: Copy + FromPrimitive + Num + Neg<Output = Self> + Sync + Send + 'static {}

enables usage of different structs that are defined outside the crate.

These changes are needed for our use case: We use own numeric types (HyperDual numbers, see num-hyperdual) to calculate derivatives of convolution integrals.

@ejmahler
Copy link
Owner

ejmahler commented Dec 4, 2020

In what way is Signed overly-restrictive? Are you straight up unable to implement that trait? Or do its methods just not make sense for your struct?

This is a breaking change - if someone depending on RustFFT wrote a function that was generic over FFTnum, and called abs() on an instance of a FFTnum, their code would no longer compile.

I suspect there are workarounds though, like making a new trait, making the FFT trait generic over that new trait, then making FFTnum require that new trait + Signed. What do you think?

@prehner prehner changed the title Change 'Signed' to 'Num + Neg' and add generic impl Add generic implementation for FFTnum Dec 4, 2020
@prehner
Copy link
Author

prehner commented Dec 4, 2020

We discussed this topic and decided that it is indeed not unreasonable to implement the Signed trait for dual numbers. (They are not differentiable at x=0, but the same is true for other functions like recip which are implemented).

Therefore it is only the generic implementation that is required to use this crate with dual numbers. This is an enormous advantage over any FFT implementation with external bindings, so thank you at this point for the work you put in this nice crate.

@ejmahler ejmahler mentioned this pull request Dec 18, 2020
@ejmahler
Copy link
Owner

Superseded by #12, mainly so I could omit the formatting changes

@ejmahler ejmahler closed this Dec 18, 2020
@ejmahler
Copy link
Owner

@prehner To let you know, I just release v4.1 on crates.io which has this change.

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.

3 participants