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

Fix #4270 #9271

Merged
merged 2 commits into from
Dec 8, 2014
Merged

Fix #4270 #9271

merged 2 commits into from
Dec 8, 2014

Conversation

simonster
Copy link
Member

This also makes squeeze require a tuple and deprecates the version that takes an iterator. I doubt there are many instances where the
dimensions to be squeezed aren't constant.

This also makes `squeeze` require a tuple and deprecates the version
that takes an iterator. I doubt there are many instances where the
dimensions to be squeezed aren't constant.
@JeffBezanson
Copy link
Member

It's a bit unsatisfying to need a staged function for something so simple. This function has always seemed marginal anyway. Do you find squeeze useful?
It'd be better if we can find a static arithmetic hack (e.g. https://gist.github.com/JeffBezanson/10612461) for this instead.

@simonster
Copy link
Member Author

Unfortunately my life is full of 4, 5, and 6 dimensional arrays. I often use squeeze because I've done something like Y = std(sum(X, 4), 1) and I need to get rid of the extra dimensions so that I can pass Y to imshow or some other plotting command.

I think we can get type inference without a staged function if we annotate the tuple passed to reshape as ::typeof(_sub(size(X), dims)). I'm not sure we could avoid heap-allocating tuples in a loop without a staged function, but maybe that doesn't matter.

@simonster
Copy link
Member Author

Here's the variant without a stagedfunction that uses _sub. (Not sure if we want to put static arithmetic in a module or if I'm okay copy/pasting that piece here.) It think it would be possible to avoid those heap-allocated tuples using recursion, but this is easier to read and if you're using squeeze in a tight loop you're probably doing something wrong.

@@ -142,20 +142,32 @@ reshape(a::AbstractArray, dims::Int...) = reshape(a, dims)
vec(a::AbstractArray) = reshape(a,length(a))
vec(a::AbstractVector) = a

function squeeze(A::AbstractArray, dims)
argtail(x, rest...) = rest
Copy link
Member

Choose a reason for hiding this comment

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

These are actually already defined elsewhere in Base.

@JeffBezanson
Copy link
Member

This is nicer; I'm fine with this version.

JeffBezanson added a commit that referenced this pull request Dec 8, 2014
@JeffBezanson JeffBezanson merged commit d7655de into master Dec 8, 2014
simonster added a commit that referenced this pull request Dec 9, 2014
simonster added a commit that referenced this pull request Dec 9, 2014
Also clean up spacing for `permutedims` while I'm here.

cc @ivarne
@jiahao jiahao deleted the sjk/squeeze branch January 18, 2015 07:24
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.

2 participants