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

RFC: make reshape always share data #15916

Closed
wants to merge 3 commits into from
Closed

RFC: make reshape always share data #15916

wants to merge 3 commits into from

Conversation

JeffBezanson
Copy link
Member

Fixes #4211. I added collect(itr, dims) to make it easier to replace copying calls to reshape. I also had to revert #15251, since squeeze calls reshape. I assume we want it to have the same behavior. If not, I can add back a copying squeeze for sparse.

@timholy
Copy link
Member

timholy commented Apr 18, 2016

A lot of this is redundant strictly worse than #15449. Why force people away from reshape and then encourage them back later?

collect{T}(::Type{T}, itr::Dims) = collect(Generator(T, itr))

"""
collect(iterator, dims)
Copy link
Contributor

Choose a reason for hiding this comment

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

needs an rst signature and a genstdlib run

@JeffBezanson
Copy link
Member Author

@timholy That's a fair point. I would be fine with merging #15449 first, then using it to significantly cut down this change. I don't mind if there are a few performance corner cases.

@timholy
Copy link
Member

timholy commented Apr 18, 2016

Good.

IIRC (it's been a while), one of the few current sources of test failure in #15449 is that it goes whole-hog and even avoids calling the C code that reshapes Arrays. That code is not deleted in that PR, but if we go whole-hog it should be. One might view this as something we really want to do: I'll point out it lays the foundation for writing Array in julia-code and having jltypes.c simply define a Buffer{T} type. Alternatively, it could be viewed as an interesting demonstration that nothing "real" broke (either method-wise or performance-wise); point made, we could go back to using the C code on Array (at least for now).

The immediate decision we have to make is this: there are some tests that explicitly check the return type of reshape([x for x in X], sz). Currently this returns an Array, but in #15449 it currently returns a ReshapedArray{Array}. There are at least three ways to fix this: (1) change the tests, (2) go back to relying on the C reshaping code for Array, or (3) define [x for x in X] to return an Array with the size of X. Which of these do you prefer?

@JeffBezanson
Copy link
Member Author

I think we will do (3) in any case. I also think we should do (2) to make the transition smoother.

@JeffBezanson JeffBezanson deleted the jb/reshape branch April 20, 2016 22:10
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.

3 participants