-
Notifications
You must be signed in to change notification settings - Fork 110
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
Deprecate multi-arg and tuple nextfastfft
#578
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #578 +/- ##
==========================================
- Coverage 97.84% 97.84% -0.01%
==========================================
Files 19 19
Lines 3252 3250 -2
==========================================
- Hits 3182 3180 -2
Misses 70 70 ☔ View full report in Codecov by Sentry. |
98e391f
to
196868a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks fine, unless somewhere down the line there's a more optimal padding heuristic that doesn't generalize so well to nD?
Ah, that's a valid point, it already shows for n=2: julia> @btime fft($(rand( 91 , 2^16)));
121.739 ms (8 allocations: 182.00 MiB)
julia> @btime fft($(rand(nextfastfft(91), 2^16)));
359.419 ms (8 allocations: 192.00 MiB) while julia> @btime fft($(rand( 91 )));
3.718 μs (6 allocations: 3.48 KiB)
julia> @btime fft($(rand(nextfastfft(91))));
3.238 μs (6 allocations: 3.48 KiB) (BTW, I have no clue why the 2d-case is that bad.) But even if we decide that a proper |
I don't see any usage of the |
Ok, so instead of deprecating, we could also document the mutli-arg and/or tuple method and point out that in the future, the result might be optimized towards nD-FFT and not just be determined point-wise. I'd be fine with both. I still tend to prefer the current PR, but just because the work's already been done. So if anyone opens a PR with the docs change, I'm happy to close this one, otherwise I'll go ahead and merge in a few days. |
Closing in favor of #580. |
Closes #291. I agree with the assessment therein and add that the remaining method is the only one documented.