change 1:ndims(X) to ntuple(identity, ndims(X)) #98
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #98 +/- ##
=======================================
Coverage 87.08% 87.08%
=======================================
Files 3 3
Lines 209 209
=======================================
Hits 182 182
Misses 27 27
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
|
Looks like |
| @@ -59,7 +59,7 @@ _to1(::Tuple, x) = copy1(eltype(x), x) | |||
| for f in (:fft, :bfft, :ifft, :fft!, :bfft!, :ifft!, :rfft) | |||
| pf = Symbol("plan_", f) | |||
| @eval begin | |||
| $f(x::AbstractArray) = $f(x, 1:ndims(x)) | |||
| $f(x::AbstractArray) = $f(x, ntuple(identity, ndims(x))) | |||
There was a problem hiding this comment.
Can we just use
| $f(x::AbstractArray) = $f(x, ntuple(identity, ndims(x))) | |
| $f(x::AbstractArray) = $f(x, Base.OneTo(ndims(x))) |
? Maybe that already fixes constant propagation? It would be less breaking (I think) and should also be more efficient if ndims(x) is large.
There was a problem hiding this comment.
I don't think this necessarily fixes the issue without the compiler propagating ndims(x) as a constant.
Regarding large ndims, IIUC Tuples are performant till a length of 32, and it's uncommon to have arrays with that many dimensions. In any case, this could be an optimization for ndims(x)<4, which is perhaps the common case.
I fully understand and sympathize with your concern about breakages. I'll see if the direct dependants can be fixed first so that they accept arbitrary regions, before this is considered
Fix #97