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

Methods defined for StridedArrays vs. AbstractArrays #1276

Closed
tshort opened this issue Sep 12, 2012 · 7 comments
Closed

Methods defined for StridedArrays vs. AbstractArrays #1276

tshort opened this issue Sep 12, 2012 · 7 comments
Labels
needs decision A decision on this change is needed

Comments

@tshort
Copy link
Contributor

tshort commented Sep 12, 2012

There are many methods in array.jl and statistics.jl that are defined based on StridedArrays. It seems like most of these could be defined based on AbstractArrays instead. Here's one example:

function prod{T}(A::StridedArray{T})
    if isempty(A)
        return one(T)
    end
    v = A[1]
    for i=2:numel(A)
        v *= A[i]
    end
    v
end

This doesn't call any C code that requires an Array. It just does indexing. Is there a reason this can't be for an AbstractArray instead?

Issue #987 somewhat relates.

@JeffBezanson
Copy link
Member

This has to do with the efficiency of linear indexing. A loop like this is a disaster for sparse arrays (though sum would be a better example there) or distributed arrays. This is actually a major problem to fix, which in fact Stefan and I were talking about just today. We need more abstract indexes, so something like this loop can work.

@tshort
Copy link
Contributor Author

tshort commented Sep 13, 2012

It might be a disaster for sparse arrays, but it will work fine for many other types of arrays (like JuliaData's DataVec). For sparse arrays and other formats where indexing like this is inefficient, you'll want to define specific methods to cover these anyway.

@tshort
Copy link
Contributor Author

tshort commented Sep 13, 2012

Also, another problem with a StridedArray is that you cannot have a type that inherits from it because it is a Union and not an abstract type. That's where issue #987 comes in.

@ViralBShah
Copy link
Member

I think we have largely sorted out the methods that should be implemented for StridedArrays vs. AbstractArrays.

#987 and #2345 continue to be open and relevant.

@tshort If DataFrames is largely ok, can we close this one?

@tshort
Copy link
Contributor Author

tshort commented Feb 27, 2013

I think that's mostly correct, Viral. I did find a few more functions that (I think) could be generalized more:

array.jl:1159:function find(testf::Function, A::StridedArray)
array.jl:1173:function find(A::StridedArray)
array.jl:1189:findn(A::StridedVector) = find(A)
array.jl:1191:function findn(A::StridedMatrix)
array.jl:1219:function findn{T}(A::StridedArray{T})
array.jl:1237:function findn_nzs{T}(A::StridedMatrix{T})
array.jl:1257:function nonzeros{T}(A::StridedArray{T})
array.jl:1443:function sum{T}(A::StridedArray{T})
array.jl:1454:function sum_kbn{T<:FloatingPoint}(A::StridedArray{T})
array.jl:1476:function cumsum_kbn{T<:FloatingPoint}(v::StridedVector{T})
array.jl:1498:function cumsum_kbn{T<:FloatingPoint}(A::StridedArray{T}, axis::Integer)
array.jl:1569:function amap(f::Function, A::StridedArray, axis::Integer)
linalg.jl:3:function scale!{T<:Number}(X::StridedArray{T}, s::Real)
linalg.jl:152:function linreg{T<:Number}(X::StridedVecOrMat{T}, y::Vector{T})
statistics.jl:76:function hist(v::StridedVector, nbins::Integer)
statistics.jl:98:function hist(A::StridedMatrix, nbins::Integer)
statistics.jl:107:function hist(v::StridedVector, edg::AbstractVector)
statistics.jl:124:function hist(A::StridedMatrix, edg::AbstractVector)

@ViralBShah
Copy link
Member

Could you open a pull request with the ones that you think we can generalize? That should lead to a speedy resolution of this issue.

ViralBShah added a commit that referenced this issue Mar 4, 2013
Change several methods with StridedArray args to AbstractArray (#1276)
@ViralBShah
Copy link
Member

Closed by d443c81

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs decision A decision on this change is needed
Projects
None yet
Development

No branches or pull requests

3 participants