Rename ProjectionStyle -> AdjointStyle, _mul -> AdjointMul, and improve docs #109

vpuri3 · 2023-07-09T02:09:24Z

This PR makes the projection styles subtypes of an abstract type. This is so that new projection styles can be added in downstream packages. In FFTW.jl, we are defining projection styles for DCT and FFTW.R2R transforms in JuliaMath/FFTW.jl#249.

@gaurav-arya can you add a few lines of documentation for the different ProjectionStyles? For example,

which fft function does each projection style correspond to?
what is the normalization scheme involved

I fear that the names are slightly confusing. Specifically, NoProjectionStyle made it seem like it would be applicable to unitary transforms. But it looks like NoProjectionStyle is applicable to fft/bfft.

…ckages. add a few lines of docs

codecov · 2023-07-09T02:19:31Z

Codecov Report

Patch coverage: 92.85% and project coverage change: -0.61 ⚠️

Comparison is base (1cc9ca0) 92.04% compared to head (bc6bee8) 91.43%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #109      +/-   ##
==========================================
- Coverage   92.04%   91.43%   -0.61%     
==========================================
  Files           3        3              
  Lines         289      292       +3     
==========================================
+ Hits          266      267       +1     
- Misses         23       25       +2

Impacted Files	Coverage Δ
src/definitions.jl	`82.99% <92.85%> (-1.04%)`	⬇️

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

gaurav-arya · 2023-07-09T02:50:50Z

It's a fair point about the name: I think it's still flexible, @devmotion @sethaxen would you agree? What about AdjointStyle rather than ProjectionStyle, since we have to write out the full adjoint def anyway for each projection style? And then just being direct, e.g. FFTAdjointStyle, RFFTAdjointStyle, IRFFTAdjointStyle, etc.

Please feel free to suggest what you think might be clearest?

Edit: We could even make the trait the function itself... i.e. we have a trait like plan_function where a plan tells us which function its trying to compute (fft, rfft, brfft, dct, etc.), and then we have adjoint dispatches for those functions like ::typeof(fft) (where we'd reuse the same underlying logic for e.g. brfft and irfft).

vpuri3 · 2023-07-09T10:32:26Z

i like the idea of renaming them FFTAdjointStyle, etc and three trait idea

devmotion · 2023-07-19T22:05:01Z

docs/src/implementations.md

-    * `AbstractFFTs.NoProjectionStyle()`,
-    * `AbstractFFTs.RealProjectionStyle()`, for plans that halve one of the output's dimensions analogously to [`rfft`](@ref),
-    * `AbstractFFTs.RealInverseProjectionStyle(d::Int)`, for plans that expect an input with a halved dimension analogously to [`irfft`](@ref), where `d` is the original length of the dimension.
+* We offer an experimental `AdjointStyle` trait to enable automatic computation of adjoint plans via [`Base.adjoint`](@ref). 


Why is the trait experimental? And what exactly is experimental? I guess downstream packages would be interested in what parts (if any) are considered stable, if every patch release may break things etc.

devmotion · 2023-07-19T22:05:44Z

docs/src/implementations.md

+  To support adjoints in a new plan, define the trait `AbstractFFTs.AdjointStyle(::MyPlan)`. This should return a subtype of `AS <: AbstractFFTs.AdjointStyle` supporting `AbstractFFTs.adjoint_mul(::Plan, ::AbstractArray, ::AS)` and 
+  `AbstractFFTs._output_size(::Plan, ::AS)`. 
+
+  `AbstractFFTs` pre-implements the following adjoint styles: [`AbstractFFTs.FFTAdjointStyle`](@ref), [`AbstractFFTs.RFFTAdjointStyle`](@ref), [`AbstractFFTs.IRFFTAdjointStyle`](@ref), and [`AbstractFFTs.UnitaryAdjointStyle`](@ref).


What is meant with pre-implements? AFAICT the package implements these styles? Or is there anything missing?

devmotion · 2023-07-19T22:06:46Z

docs/src/implementations.md

-    * `AbstractFFTs.RealProjectionStyle()`, for plans that halve one of the output's dimensions analogously to [`rfft`](@ref),
-    * `AbstractFFTs.RealInverseProjectionStyle(d::Int)`, for plans that expect an input with a halved dimension analogously to [`irfft`](@ref), where `d` is the original length of the dimension.
+* We offer an experimental `AdjointStyle` trait to enable automatic computation of adjoint plans via [`Base.adjoint`](@ref). 
+  To support adjoints in a new plan, define the trait `AbstractFFTs.AdjointStyle(::MyPlan)`. This should return a subtype of `AS <: AbstractFFTs.AdjointStyle` supporting `AbstractFFTs.adjoint_mul(::Plan, ::AbstractArray, ::AS)` and 


I.e., adjoint_mul is part of the API?

devmotion · 2023-07-19T22:07:45Z

docs/src/implementations.md

-    * `AbstractFFTs.RealInverseProjectionStyle(d::Int)`, for plans that expect an input with a halved dimension analogously to [`irfft`](@ref), where `d` is the original length of the dimension.
+* We offer an experimental `AdjointStyle` trait to enable automatic computation of adjoint plans via [`Base.adjoint`](@ref). 
+  To support adjoints in a new plan, define the trait `AbstractFFTs.AdjointStyle(::MyPlan)`. This should return a subtype of `AS <: AbstractFFTs.AdjointStyle` supporting `AbstractFFTs.adjoint_mul(::Plan, ::AbstractArray, ::AS)` and 
+  `AbstractFFTs._output_size(::Plan, ::AS)`. 


I assume _output_size should be implemented as well? Could we just rename it to output_size? To me the underscore suggests that it is an internal method.

gaurav-arya · 2023-07-19T23:27:38Z

Thanks for the comments, I've revised the PR.

src/definitions.jl

gaurav-arya · 2023-07-21T15:26:01Z

Right, fixed.

gaurav-arya · 2023-07-27T01:24:55Z

Bump on this or #78

devmotion

Looks good to me.

gaurav-arya · 2023-07-27T13:16:28Z

Thanks! I don't have merge privileges so please merge when ready 🙂 (after merge I can fix merge conflicts in #78)

make ProjectionStyle abstract type so we can subtype in downstream pa…

d53f57d

…ckages. add a few lines of docs

gaurav-arya mentioned this pull request Jul 18, 2023

Tracking issue for plan chain rule support #94

Open

5 tasks

gaurav-arya force-pushed the proj branch from 93a8102 to bfd3133 Compare July 18, 2023 17:22

Rename ProjectionStyle -> AdjointStyle, _mul -> AdjointMul, improve docs

f3575aa

gaurav-arya force-pushed the proj branch from ae38a0c to f3575aa Compare July 18, 2023 20:44

vpuri3 changed the title ~~make ProjectionStyle abstract type~~ Rename ProjectionStyle -> AdjointStyle, _mul -> AdjointMul, and improve docs Jul 18, 2023

Clarify normalization

00eef4f

This was referenced Jul 19, 2023

Support new AbstractFFTs.AdjointStyle trait for FFT and DCT plans JuliaMath/FFTW.jl#249

Merged

Put tests of FFT backends into TestUtils submodule #78

Merged

devmotion reviewed Jul 19, 2023

View reviewed changes

gaurav-arya force-pushed the proj branch from e018be1 to f865f30 Compare July 19, 2023 23:07

Clarify documentation, rename _output_size -> output_size

17adac8

gaurav-arya force-pushed the proj branch from f865f30 to 17adac8 Compare July 19, 2023 23:09

gaurav-arya added 3 commits July 19, 2023 22:31

Remove unnecessary def

bbd9f41

Remove confusing commas

9b724ed

Tweak docstring wording

d5d7ce2

devmotion reviewed Jul 20, 2023

View reviewed changes

src/definitions.jl Outdated Show resolved Hide resolved

gaurav-arya added 2 commits July 20, 2023 22:03

Reposition and improve size/output_size docstrings

eddf7e3

Note that size needs to be implemented in docs

bc6bee8

devmotion approved these changes Jul 27, 2023

View reviewed changes

devmotion merged commit 5c23f4b into JuliaMath:master Jul 27, 2023

vpuri3 deleted the proj branch January 18, 2024 13:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rename ProjectionStyle -> AdjointStyle, _mul -> AdjointMul, and improve docs #109

Rename ProjectionStyle -> AdjointStyle, _mul -> AdjointMul, and improve docs #109

vpuri3 commented Jul 9, 2023 •

edited

Loading

codecov bot commented Jul 9, 2023 •

edited

Loading

gaurav-arya commented Jul 9, 2023 •

edited

Loading

vpuri3 commented Jul 9, 2023

devmotion Jul 19, 2023

devmotion Jul 19, 2023

devmotion Jul 19, 2023

devmotion Jul 19, 2023

gaurav-arya commented Jul 19, 2023

gaurav-arya commented Jul 21, 2023

gaurav-arya commented Jul 27, 2023

devmotion left a comment

gaurav-arya commented Jul 27, 2023 •

edited

Loading

Rename ProjectionStyle -> AdjointStyle, _mul -> AdjointMul, and improve docs #109

Rename ProjectionStyle -> AdjointStyle, _mul -> AdjointMul, and improve docs #109

Conversation

vpuri3 commented Jul 9, 2023 • edited Loading

codecov bot commented Jul 9, 2023 • edited Loading

Codecov Report

gaurav-arya commented Jul 9, 2023 • edited Loading

vpuri3 commented Jul 9, 2023

devmotion Jul 19, 2023

Choose a reason for hiding this comment

devmotion Jul 19, 2023

Choose a reason for hiding this comment

devmotion Jul 19, 2023

Choose a reason for hiding this comment

devmotion Jul 19, 2023

Choose a reason for hiding this comment

gaurav-arya commented Jul 19, 2023

gaurav-arya commented Jul 21, 2023

gaurav-arya commented Jul 27, 2023

devmotion left a comment

Choose a reason for hiding this comment

gaurav-arya commented Jul 27, 2023 • edited Loading

vpuri3 commented Jul 9, 2023 •

edited

Loading

codecov bot commented Jul 9, 2023 •

edited

Loading

gaurav-arya commented Jul 9, 2023 •

edited

Loading

gaurav-arya commented Jul 27, 2023 •

edited

Loading