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: Return views from UnitRange indexing of Arrays #9150

Closed
wants to merge 3 commits into from

Conversation

andreasnoack
Copy link
Member

This PR changes the behavior of array indexing to return a SubArray of the original array instead of an Array. This should reduce the number of intermediate arrays allocated in computations and hopefully that change will not be notified much in casual command prompt use of Julia.

I have decided to use slice instead of sub which is a change in indexing convention. However, this can easily be changed in the pull request if we decide to stick with the present convention. Because of this, I haven't adjusted the arrayops.jl tests yet. I'll only do that when we have decided to use either sub or slice. The rest of the tests should have been through the necessary adjustments.

Examples:

julia> A = randn(4,4)
4x4 Array{Float64,2}:
  0.735034   2.32154     1.40147    -1.6871  
 -0.683693   0.72711     0.105667    1.30328 
  1.03254    0.038653   -0.0259387  -1.46718 
  0.600892  -0.0290964  -1.52698    -0.194185

julia> A[1:3,1:3]
3x3 SubArray{Float64,2,Array{Float64,2},(UnitRange{Int64},UnitRange{Int64}),1}:
  0.735034  2.32154    1.40147  
 -0.683693  0.72711    0.105667 
  1.03254   0.038653  -0.0259387

julia> A[1,:]
4-element SubArray{Float64,1,Array{Float64,2},(Int64,Colon),2}:
  0.735034
  2.32154 
  1.40147 
 -1.6871  

@carlobaldassi I have made some adjustments to the BitArray tests because indexing Array{Bool} now behaves differently. I haven't changed the behavior of BitArray indexing.

I have also not made any changes to indexing of SparseMatrixCSC.

@JeffBezanson I had to change quite a few places in inference.jl to avoid the creation of SubArrays there. You might want to take a look and see if the changes there are okay.

@timholy
Copy link
Member

timholy commented Nov 25, 2014

Exciting stuff! I'll try to find some time over the holiday to review this. Really great to see this come along so quickly!

@@ -360,7 +360,7 @@ function show_call(io::IO, head, func, func_args, indent)
end
if !isempty(func_args) && isa(func_args[1], Expr) && func_args[1].head === :parameters
print(io, op)
show_list(io, func_args[2:end], ',', indent, 0)
show_list(io, [func_args[i] for i = 2:length(func_args)], ',', indent, 0)
Copy link
Member

Choose a reason for hiding this comment

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

I'm not clear on why any show methods would need a copy.

Copy link
Member Author

Choose a reason for hiding this comment

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

After your latest change that returns FastLinear more often these might not be necessary, but before they returned SlowLinear for which iteration was not defined until multidimensional.jl or cartesian.jl. I'll try slicing and see if it works.

Copy link
Member

Choose a reason for hiding this comment

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

Got it. #9098 is progress towards the goal of moving cartesian.jl and the iterators part of multidimensional.jl earlier in the boot sequence, but currently it's stuck due to #8504. So in the meantime a copy, if necessary, seems fine.

@timholy
Copy link
Member

timholy commented Dec 1, 2014

Looks really good. Obviously, the Travis failure is the main thing to be concerned about. The other thing I'd ask is whether you just focused on test failures, or whether you also searched through base to find uses of Array, Matrix, and Vector which might need to be changed to StridedArray etc.

@andreasnoack
Copy link
Member Author

I'd ask is whether you just focused on test failures, or whether you also searched through base to find uses of Array, Matrix, and Vector which might need to be changed to StridedArray etc.

No I haven't done that systematically, but I a taught some because of the tests.

I'll have to spend some more time on making the Colon go through correctly and maybe I'll wait with that work until you are done with your work on cartesian.jl.

@StefanKarpinski Would you like to express your view on the slice vs. sub behavior. This is related to JuliaLang/LinearAlgebra.jl#42. If we want to change getindex behavior from sub-style to slice-style, I think it would be easier to make the change together with introducing views from getindex. Then we'll avoid two breaking changes to the getindex behavior.

@tknopp
Copy link
Contributor

tknopp commented Feb 1, 2015

Given that this is a very important semantic change for 0.4 it would be good getting this in rather earlier than later.

@timholy
Copy link
Member

timholy commented Feb 1, 2015

Everything depends on resolving #8504.

@tknopp
Copy link
Contributor

tknopp commented Feb 1, 2015

Ah I see. Hopefully this gets resolved because IMHO this is an important change for 0.4, which I am really looking forward to

andreasnoack added a commit that referenced this pull request Feb 25, 2015
Cherry-picked from a squashed-and-split version of #9150 to exclude the bigger change of returning views from indexing with UnitRanges.

Conflicts:
	base/abstractarray.jl
	base/multidimensional.jl
	src/julia-syntax.scm
andreasnoack added a commit that referenced this pull request Mar 5, 2015
Cherry-picked from a squashed-and-split version of #9150 to exclude the bigger change of returning views from indexing with UnitRanges.

Conflicts:
	base/abstractarray.jl
	base/multidimensional.jl
	src/julia-syntax.scm
andreasnoack added a commit that referenced this pull request Mar 6, 2015
Cherry-picked from a squashed-and-split version of #9150 to exclude the bigger change of returning views from indexing with UnitRanges.

Conflicts:
	base/abstractarray.jl
	base/multidimensional.jl
	src/julia-syntax.scm
@GravityAssisted
Copy link

So we are planning to use Julia for space trajectory design package here at JPL and one of the thing which is hurting us is this issue of having array return slices as views.

For now, we have decided to use the excellent ArrayViews package, but I wanted to know if there is a plan to address this issue anytime soon (by early 2017) ?

thanks,
Nitin

@mweastwood
Copy link
Contributor

@GravityAssisted array views are already part of Julia v0.4

x = randn(5, 5)
x[1:2, 3:4] # makes a copy
sub(x, 1:2, 3:4) # makes a view

The syntax isn't quite as convenient, but if you intend to exclusively use views there is nothing stopping you from creating an array wrapper that defines getindex to call sub.

If you need additional assistance you should contact the julia-users mailing list, which is a more suitable location for these kinds of questions.

@StefanKarpinski
Copy link
Member

This syntax has been proposed for making views:

x[1:2, 3:4] # copy
x@[1:2, 3:4] # view

@JaredCrean2
Copy link
Contributor

@GravityAssisted note that sub has non-trivial overhead for small arrays (ArrayViews.view does too, but ArrayViews.unsafe_view does not, at the cost of no bounds checking). There is hope this will go away in the future, but in the meantime, you might want to include something like this in your code:

global const use_arrayviews = true
if use_arrayviews
  global const sview = ArrayViews.unsafe_view
else
  global const sview = sub
end

and use sview in place of sub, view, or unsafe_view. These all have identical syntax, so its easy to switch between them, and there is no overhead from using this construct.

@andreasnoack
Copy link
Member Author

andreasnoack commented Aug 12, 2016

It might be worth mentioning that the unsafe_view is prefixed unsafe for a reason. If you use it then your code might cause Julia to segfault.

@GravityAssisted
Copy link

Thanks all for the quick response. I ended up using unsafe_view for now, as suggested by @JaredCrean2 . It will be good to have a non-allocating solution as part of the native language. As suggested by @mweastwood , I will move this issue to julia-users for the future.

@ChrisRackauckas
Copy link
Member

ChrisRackauckas commented Aug 15, 2016

@andreasnoack is there much of a difference between unsafe_view and @inbounds with view or sub? Just wondering because I'd hope @inbounds x@[1:2,3:4] would dig down to the most efficient view.

@andreasnoack
Copy link
Member Author

The inefficiency related to safe views is not about bounds checking but the memory allocation required by the view. If an immutable only contains fields of other immutables like e.g. Float64 and Tuple{Int,Int} then it can be efficiently allocated but if it has fields with e.g. an Array then it cannot be efficiently allocated (yet). The unsafe view gets around this by having a Ptr{T} field instead of an Array{T,N} field. Ptr is similar to Float64 and Int and other immutables. However, the GC doesn't know that the Ptr field points to an array so it's up to you to make sure that the view is not used after the parent array has been freed and therefore it's an unsafe operation.

@ChrisRackauckas
Copy link
Member

ChrisRackauckas commented Aug 15, 2016

Thanks for the explanation. Is there plans to handle pointers allocation better? It seems like that would be necessary for applications like defining your own variable length bit types (like unums). And if so, would this inefficiency then be handled?

@andreasnoack
Copy link
Member Author

I think it is but it's also (I've been told) quite difficult. Stefan talks a bit about it in his JuliaCon talk so if you haven't watched it already it might be worth doing.

@simonbyrne
Copy link
Contributor

simonbyrne commented Jul 20, 2017

Given the discussion in #3701 and JuliaLang/LinearAlgebra.jl#255, should this now be closed?

@andreasnoack
Copy link
Member Author

Yes

@andreasnoack andreasnoack deleted the anj/getindex branch July 23, 2017 18:48
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.

9 participants