-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
Support non-1 indices and fix type problems in DFT (fixes #17896) #17919
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
Conversation
I'm glad you're pushing this forward, @timholy! Any change that shouldn't break code and isn't too risky should probably be marked for backporting to release-0.5. Does this qualify? |
Minus the export, I should add. |
Yes, it qualifies. Thanks for the reminder re the label.
Nothing like having a new real-world application to put it through its paces...the application I used while developing non-1 indices in the first place was based on old code where I faked unconventional indices, but perforce that couldn't stretch the limits like it's possible to do now. Once some stuff works, you quickly discover how much more you want to get working. |
@@ -662,6 +662,66 @@ function _circshift!(dest, rdest, src, rsrc, inds, shiftamt) | |||
copy!(dest, CartesianRange(rdest), src, CartesianRange(rsrc)) | |||
end | |||
|
|||
# circcopy! | |||
""" | |||
circcopy!(dest, src) |
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.
for 0.6 this should go in the rst manual if exported
I split the I also added comments as requested. |
3 7 11 15 | ||
4 8 12 16 | ||
|
||
julia> dest = OffsetArray(Int, (-1,1)); |
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.
this doctest won't pass without some setup code to load the OffsetArray type definition from test/
, will it?
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.
Good catch. I'm guessing we can't just say using OffsetArrays
, for reasons previously discussed. Is there some kind of "hidden" setup possible? Or should I just remove the doctest and make sure this runs in the main tests?
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.
I believe there is a way to do a few lines of test setup that don't appear in the rendered docs. It might be worth moving the definition of the type and methods from test/offsetarray.jl
to test/TestHelpers.jl
, if the rest of the correctness tests take a little too long to run as part of the doctest setup.
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.
I just got rid of it, as (1) I was not successful in figuring out hiding, and (2) the printing of the typename just didn't seem to benefit from having TestHelpers in it. I did move the OAs module to TestHelpers.jl, however.
I think this can be merged once CI passes. |
where it is not exported, ref #17919
This supports FFT on non-1 arrays, by introducing
circcopy!
:As a consequence, index information gets preserved in the phase of the FFT. I tried an alternative implementation, multiplying by phases post hoc, but in my hands that was slower.
I also took the opportunity to fix (and test for) #17896.
CC @stevengj.