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

Generalize scalar indexing and implement reducedim without stagedfunctions #10524

Merged
merged 4 commits into from
Mar 18, 2015

Conversation

timholy
Copy link
Member

@timholy timholy commented Mar 15, 2015

This generalizes CartesianIndex indexing to allow a construct like A[i::Int,I::CartesianIndex]. This allows one to split out the innermost loop for special treatment. We did that already for the special case of @simd, this just generalizes it in a way that allows one to exploit it for pure julia code, too.

CC @Jutho, @mbauman. Some day we probably will want to go further, and allow full getindex(A, indexes::Union(Integer, CartesianIndex)...). But for now we can't add that without generating ambiguity warnings, and I don't currently need the general case for anything.

I then used this to rewrite the algorithms in reducedim.jl without using Base.Cartesian or any staged functions, with no loss of performance. (In fact, the sum!(R2, A) case is about 15% faster.)

 julia> A = rand(10^4, 10^4);

julia> R1 = zeros(1, size(A,2));   # for reducing along dimension 1

julia> R2 = zeros(size(A,1), 1);   # for reducing along dimension 2

julia> R12 = zeros(1, 1);       # for reducing along dimensions 1 and 2

julia> @time sum!(R1, A);
elapsed time: 0.350055697 seconds (4 MB allocated)

julia> @time sum!(R2, A);
elapsed time: 0.188841453 seconds (144 bytes allocated)

julia> @time sum!(R12, A);
elapsed time: 0.128383574 seconds (144 bytes allocated)

julia> @time sum!(R1, A);
elapsed time: 0.131577355 seconds (144 bytes allocated)

julia> @time sum!(R2, A);
elapsed time: 0.18379862 seconds (144 bytes allocated)

julia> @time sum!(R12, A);
elapsed time: 0.129456885 seconds (144 bytes allocated)

julia> B = sub(A, 1:size(A,1), 1:size(A,2));  # defeat fast linear indexing

julia> Base.linearindexing(B)
Base.LinearSlow()

julia> @time sum!(R1, B);
elapsed time: 0.598274799 seconds (7 MB allocated, 3.22% gc time in 1 pauses with 1 full sweep)

julia> @time sum!(R1, B);
elapsed time: 0.125478658 seconds (144 bytes allocated)

julia> @time sum!(R2, B);
elapsed time: 0.421974903 seconds (144 bytes allocated)

julia> @time sum!(R12, B);
elapsed time: 0.125809267 seconds (144 bytes allocated)

Reference: #10310 (comment) and the comments above (this makes the "blows out of the water" comment no longer apply).

# - If it reduces only along leading dimensions, e.g. sum(A, 1) or sum(A, (1, 2)),
# it returns the length of the leading slice. For the two examples above,
# it will be size(A, 1) or size(A, 1) * size(A, 2).
# - Otherwise, e.g. sum(A, 2) or sum(A, (1, 3)), it returns 0.
#
ndims(R) <= ndims(A) || throw(DimensionMismatch("Cannot reduce $(ndims(A))-dimensional array to $(ndims(R)) dimensions"))
Copy link
Member Author

Choose a reason for hiding this comment

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

This is necessary to prevent a stackoverflow from size_skip1 if ndims(R) > ndims(A). I doubt that it used to be an error. I don't think there's any good reason to support it, but if others disagree, then size_skip1 needs to be a stagedfunction.

@Jutho
Copy link
Contributor

Jutho commented Mar 15, 2015

The CartesianIndex changes look great to me. I have not really checked the reducedim.jl changes as I am not familiar with this code.

@timholy timholy force-pushed the teh/comboindexing branch from c4d0f54 to 4cdd5bc Compare March 17, 2015 11:00
timholy added 4 commits March 17, 2015 12:54
Eventually we probably want to support
    getindex(A, indexes::Union(Integer, CartesianIndex)...),
but for now that causes ambiguity warnings.
This pushes the stagedfunction operations to a lower level,
indexing, and lets us write more complex algorithms using
standard julia syntax.
@timholy timholy force-pushed the teh/comboindexing branch from 4cdd5bc to 705efdc Compare March 17, 2015 17:54
timholy added a commit that referenced this pull request Mar 18, 2015
Generalize scalar indexing and implement reducedim without stagedfunctions
@timholy timholy merged commit 5762a5d into master Mar 18, 2015
@timholy timholy deleted the teh/comboindexing branch March 18, 2015 19:42
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