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

Size included in plans #18

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

milankl
Copy link
Collaborator

@milankl milankl commented Sep 2, 2022

Certainly won't pass CI hence the draft. FFTW includes sz, the size of the input array and osz the size of the output arrays into their plans. So I tried to mimick that here too. For some plans sz==osz but not for all. Still have to work on this, but @daanhb is this roughly what you suggested in #10 ?

@daanhb
Copy link
Member

daanhb commented Sep 2, 2022

To be honest, I'm not familiar with the FFTW plans. I'm curious about cases where osz does not equal sz, that is unexpected to me. What I had in mind was:

  • indeed to fix inverse of bfft plan not supported #10. Since AbstractFFTs.jl provides a ScaledPlan, it would be great if we could use that
  • Also, if you make a plan for a vector of a certain size, then it would be nice if the transform throws an error when the plan is applied to a vector of the wrong size (or wrong element type, for that matter)
  • and I thought that the size might be needed for multidimensional transforms, but I'm not really sure about that

$f(x::AbstractArray{<:AbstractFloats}) = $pf(x) * x
$f(x::AbstractArray{<:AbstractFloats}, region) = $pf(x, region) * x
$f(x::AbstractArray{<:AbstractFloats}) = $pf(x, size(x)) * x
$f(x::AbstractArray{<:AbstractFloats}, region) = $pf(x, size(x), region) * x
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would try to avoid changing the interface of the plan_X functions. Here, you're adding size(x) as a second argument, but AbstractFFTS adds the region as a second argument here. It is the construct of the plan object that takes X as an argument and then computes its size, see e.g. here. It seems safer to mimick that?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense, yes. So calculate size(x) in the contructor and probaly infer the (if different) output size osz from that. In general, in the Fourier-transformed dimensions real-to-complex and complex-to-real it should be just n÷2+1, so a 64-element input has a 33-element complex output and vice versa.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, so different sizes are due to complex-real differences. Ok.

@daanhb
Copy link
Member

daanhb commented Sep 2, 2022

Thanks for doing this, by the way!

I've added a comment in the code. As a general principle, I think it is best to maximally reuse whatever AbstractFFTs.jl provides (in particular any defaults), and mimick what FFTW does. Unless something seems illogical, of course.

@milankl
Copy link
Collaborator Author

milankl commented Sep 2, 2022

No worries, we're working on a fully type-flexible weather model for which we want to understand what happens if we use half-precision in our code. At the moment only the FFT is the remaining bottleneck. Hence, I'd love to see this package supporting the stuff we need and hopefully getting a bit faster too. I believe there's a lot of allocations one could skip and maybe also scatter some @inbounds...

@daanhb
Copy link
Member

daanhb commented Sep 2, 2022

If performance is a concern, you may also want to check out FourierTransforms.jl. I don't know how the packages compare and I'm not sure what the state of the latter is, but I believe it originally started out as a Julia implementation of the fftw code (see e.g. JuliaMath/AbstractFFTs.jl#32). For best performance that should be the way to go. Right now, in this package we are completing the interface mainly to have something that works. Perhaps others have more to say on this.

Anyway, before spending time optimizing, it is worth questioning what to optimize or where we want to end up. Of course low hanging fruit is fine.

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