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

Separate types for rFFT and FFT plans (and issues with ScaledPlans) #63

Open
maximilian-gelbrecht opened this issue Jan 24, 2022 · 6 comments

Comments

@maximilian-gelbrecht
Copy link

Since PR #58 is now merged, I would suggest talking about how to extend this to Plans as well.

One way of doing that would be to implement different (abstract) types for a FFT, rFFT and r2r. This of course would have some implications for the libraries relying on AbstractFFTs. That's why I did not directly proceed with a PR, but would want to discuss this first.

Something reasonable could be to have an AbstractFFTPlan, AbstractrFFTPlan, AbstractR2RPlan that are all subtypes of the current Plan type, so that it is not a completely breaking change. (Ideally Plan should be renamed to AbstractPlan, but this would break a lot, I guess)

Aside from this, there is also JuliaMath/FFTW.jl#182 , so that ScaledPlans miss the region subfield, which could be mitigated by a function like

region(p::Plan) = p.region
region(p::ScaledPlan) = region(p.p)
@gaurav-arya
Copy link
Contributor

gaurav-arya commented Jun 3, 2022

Based on discussion with @stevengj, an alternate solution would be to define AbstractFFTs.transpose and require subtypes of Plan provide an implementation of it.

However, on second thought I'm unsure whether there is any benefit to avoiding modifying the type hierarchy as @maximilian-gelbrecht suggested? That should not be any more breaking, because nothing would dispatch on the new abstract types except the rrule's, and those require some downstream changes in any case. And it seems to make things a lot simpler, unless there is some reason why the implementation of AbstractFFTs.transpose would differ for different subtypes?

@stevengj
Copy link
Member

stevengj commented Jun 3, 2022

The reason to implement transpose is that this is the operation it seems you really want for the plan object, and it is generally useful independent of AD.

(And I would just implement methods for Base.transpose, not define a new generic function of the same name.)

@gaurav-arya
Copy link
Contributor

gaurav-arya commented Jun 3, 2022

That makes sense -- then it seems there are two options:

  1. Not add any more types and ask downstream libraries to write e.g. Base.transpose(::MyPlan, ...) = AbstractFFTs.r2r_transpose(::MyPlan, ...), where we've defined some utility functions in AbstractFFTs.

  2. Implement the type hierarchy suggested by @maximilian-gelbrecht (or traits?), and ask downstream libraries to adjust the type their plans' subtypes, where we've defined e.g. Base.transpose(::AbstractR2RPlan, ...) in AbstractFFTs.

In both cases, the rrule would then just use Base.transpose. Which of these options would you recommend?

@stevengj
Copy link
Member

stevengj commented Jun 4, 2022

The only problem with the abstract-plan approach is similar to what I commented in #56 — do we want to specify FFTW's choices for r2r transformations here?

@gaurav-arya
Copy link
Contributor

Hey, I made a draft PR here: #67

@devmotion
Copy link
Member

Aside from this, there is also JuliaMath/FFTW.jl#182 , so that ScaledPlans miss the region subfield, which could be mitigated by a function like

I opened #65 a while ago to fix this issue.

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

4 participants