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

allow iterables in fftshift and ifftshift #16229

Closed
wants to merge 4 commits into from

Conversation

JKrehl
Copy link
Contributor

@JKrehl JKrehl commented May 6, 2016

In the case of shifting multiple but not all axis of a multidimensional array now requires multiple fftshift commands (which, if I am correct, includes multiple array copys). The map-function is abstract enough to allow both iterables and scalar arguments, so I see no reason to not include this behaviour.

Please excuse any misuse of GIT and its functions.

JKrehl added 2 commits May 6, 2016 11:35
In the case of shifting multiple but not all axis of a multidimensional array now requires multiple fftshift commands (which, if I am correct, includes multiple array copys). The map-function is abstract enough to allow both iterables and scalar arguments, so I see no reason to not include this behaviour.
I should not enter code through the online interface, however small the change.
@ViralBShah
Copy link
Member

Cc @stevengj

@tkelman
Copy link
Contributor

tkelman commented May 6, 2016

Thanks for the contribution! Would you mind adding a test or two for this new functionality?

@JKrehl
Copy link
Contributor Author

JKrehl commented May 6, 2016

Certainly
Are these test sufficient?

Since this functionality is quite abstract and devolves to map's abilities to interpret the argument it would be unnecessary to test every probable use-case.

@stevengj
Copy link
Member

stevengj commented May 6, 2016

LGTM.

@@ -345,14 +345,14 @@ fftshift(x)

function fftshift(x,dim)
s = zeros(Int,ndims(x))
s[dim] = div(size(x,dim),2)
map(i -> s[i] = div(size(x,i),2), dim)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to be a very strange use of map.

@JKrehl
Copy link
Contributor Author

JKrehl commented May 6, 2016

The map rather blatenly replaces a for-loop and was chosen simply for brevity of the code. The instruction itself is so simple that the visual structuring by a for-loop is, in my opinion, not needed.

The only difference between the two in interpreting iterable arguments are strings. In the end, it doesn't matter to the use-cases I can envisage so I don't care whether it is a map or a for-loop.

If that is the aspect you are aiming at.

@yuyichao
Copy link
Contributor

yuyichao commented May 6, 2016

map allocates an array to store the result.

@stevengj
Copy link
Member

stevengj commented May 6, 2016

(You could also call foreach, which unlike map does not allocate an array.)

@JKrehl
Copy link
Contributor Author

JKrehl commented May 6, 2016

Yes, it does. Although a few small arrays of nothingness should not hurt, they are still needless allocations.

Thank, I was not aware of the existence of foreach.
Moving to a for-loop also eliminates the abstract function, for whatever that is worth.

@JKrehl JKrehl closed this Feb 3, 2017
@JKrehl JKrehl deleted the JKrehl-fftshift branch February 3, 2017 08:54
@tkelman
Copy link
Contributor

tkelman commented Feb 3, 2017

Looks like we just lost track of this one, was there any objection to it?

@StefanKarpinski
Copy link
Member

cc: @stevengj – seems like a quick review for you might do here

@stevengj
Copy link
Member

stevengj commented Feb 4, 2017

LGTM.

@StefanKarpinski
Copy link
Member

Looks like we can't merge this on GitHub since the branch is gone. I guess we should manually reconstruct this and commit it (with credit to @JKrehl), unless @JKrehl is willing to do so.

@JKrehl
Copy link
Contributor Author

JKrehl commented Feb 4, 2017

Can do: https://github.com/JKrehl/julia/tree/JKrehl/fftshift
Does it need to have the exactly same name as the original one? Or can someone change the branch attached to the PR?

(I'm slowly getting the hang of this branching-thingy.)

@tkelman
Copy link
Contributor

tkelman commented Feb 5, 2017

restoring the original branch name might let us reopen this pr. otherwise we could open a new one.

@JKrehl
Copy link
Contributor Author

JKrehl commented Feb 5, 2017

reopened it in #20461
Not having deleted the branch via GitHub's web-interface GitHub considers it gone for good.

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

Successfully merging this pull request may close these issues.

6 participants