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

Move generic FFT into its own package? #172

Closed
cgeoga opened this issue May 3, 2022 · 11 comments
Closed

Move generic FFT into its own package? #172

cgeoga opened this issue May 3, 2022 · 11 comments

Comments

@cgeoga
Copy link

cgeoga commented May 3, 2022

The generic Bluestein-based FFT in this package is great, and I use it often. But this package has a lot of heavy dependencies that I don't really want to add to my projects, and the FFT is very often the only functionality I use from it. Would it make sense to potentially move/mirror it to its own package, or potentially even put it in AbstractFFTs.jl? As it stands, to avoid cluttering the dependency tree of my projects I even sometimes just copy that single file (with proper attribution, to be clear).

If the idea is not objectionable but you don't feel like doing it, would you object to somebody else (for example me) copying it and creating, say, Bluestein.jl?

I see an existing issue (#86) asking something similar, where the proposed solution is that FourierTransforms.jl will provide nice multidimensional transforms. But that package isn't registered and hasn't seen a commit since 2019. While eventually that seems like a very natural home for such functionality, maybe something that is registered and offers something lighter in the mean time would make sense.

@dlfivefifty
Copy link
Member

I think we'd definitely support this

@cgeoga
Copy link
Author

cgeoga commented May 3, 2022

Hey, excellent. What makes the most sense as far as execution? Would you like me (or some other volunteer that isn't one of the main devs here) to make the repo and set up tests and stuff and then transfer it (?) to JuliaApproximation or something? To be honest I don't have a great understanding of the fftBigFloat.jl file and so it would be a pretty minimal copy-paste job if I did it.

Maybe @mfherbst has an opinion as well?

@dlfivefifty
Copy link
Member

Yes exactly. I don't think me or @MikaelSlevinsky have time to do this ourselves so just needs someone to take the initiative.

I don't have strong opinions on where it ends up.

@mfherbst
Copy link
Contributor

Sorry guys for the silence on my end, have been pretty busy lately.

Yes having a generic FFT routine in a separate package would be very much appreciated on my end. In DFTK we currently use a local copy of FourierTransforms.jl for generic FFTs, which is a bit buggy, so indeed any motion to revive that package and cure its bugs would be awesome.

Structure-wise I guess the suggested "outsourcing" of the generic FFT of this package to FourierTransforms.jl, which implements the AbstractFFT.jl interface sounds ideal to me.

@milankl
Copy link

milankl commented Jun 23, 2022

Just saying that there's also interest on our side, as we are looking for a generic FFT for use in SpeedyWeather.jl. Very happy to contribute, but I currently don't have an overview of what's needed where. If someone could come up with a plan what's needed, happy to execute some of the steps in it.

@dlfivefifty
Copy link
Member

It just takes someone to take the lead, which would include deciding "what's needed where". @mfherbst gave a reasonsable proposal.

@daanhb
Copy link
Member

daanhb commented Jul 18, 2022

I'd be a happy user of a separate package for generic FFT's

@daanhb
Copy link
Member

daanhb commented Jul 19, 2022

I went ahead and created a GenericFFT package: https://github.com/JuliaApproximation/GenericFFT.jl
I've added everyone in this discussion as potential collaborator.

This is just a way to get started. I copied the code from fftbigfloats.jl and added some tests for the Double64 type from DoubleFloats.jl (which by my experiments currently seems the most performant 128 bit floating point number type).

My own packages work with GenericFFT.jl as a drop-in replacement for the dependency on FastTransforms.jl. Can anyone interested try it out with your own code and report any issues you find? We can make progress that way.

It seemed more difficult to integrate with FourierTransforms.jl, but I have no strong feelings about where this code ends up - as long as I can use fft and dct routines with arbitrary floating point numbers I'll be happy. Hopefully sooner or later someone becomes interested in optimizing the transforms.

@cgeoga
Copy link
Author

cgeoga commented Jul 19, 2022

Thanks! Sorry to have been radio silent on this for so long. I appreciate the initiative you took for it. I have a couple comments/questions, but I'll open them as issues on that page.

@daanhb
Copy link
Member

daanhb commented Jul 23, 2022

The GenericFFT.jl package has been registered at version 0.1.0. There were some tweaks to dependencies, but the transform itself is the exact same code as it was here in FastTransforms so it should be functionally equivalent for everyone who used FastTransforms for that purpose.

FastTransforms extended DSP.conv for general AbstractFloat types in terms of the FFT. That part was removed from GenericFFT, in order not to depend on DSP. An issue was filed to fix this upstream so hopefully in the future 'conv' will just work.

@daanhb
Copy link
Member

daanhb commented Jul 23, 2022

Thanks everyone here for the quick responses!

@daanhb daanhb closed this as completed Jul 23, 2022
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

No branches or pull requests

5 participants