Yeppp.jl icon indicating copy to clipboard operation
Yeppp.jl copied to clipboard

Added support for views and subarrays, updated tests, added sumabs for Float32

Open mfalt opened this issue 5 years ago • 1 comments

I have found this package quite useful, but it seems to lack support for common types such as views of arrays. Hope you like it.

Notable things:

  • I added _iscontiguous to verify is a StridedArray is contiguous, and a fallback for StridedArray to false, there are probably better ways to check this? Maybe someone with better knowledge of the julia internals can comment on this?

  • Removed the argument specification Array on add and subtract. They were inconsistent with the other wrappers. Maybe they should all be Any as most of them were, or something a bit more stringent?

  • Added sumabs for Float32. I was not able to find the source to verify that this should work, but it seems to pass the tests.

  • Kept the requirement on arguments being vectors for dot, this is not the case in Base, so maybe it can be relaxed (but this is an orthogonal problem)

  • Updated evalpoly.jl to work on julia 1.0, although this code is not run automatically on tests (could also be separate issue)

EDIT: Tests fails on osx, I am not sure what is going on here, is it a build problem? Last test was run 8 months ago.

mfalt avatar Apr 04 '19 23:04 mfalt

I have been looking a bit more into this. Again, I'm not sure about any of this, the interface is a bit confusing.

It seems like the most general way to do this (we might not even case about catching all these cases), would to use the definitions from https://github.com/JuliaLang/julia/commit/9d5a05ef792e32cec8de8d09866e176ea8eaf158#diff-8b7e79ba9dcb85eae4867125aa0b8825R48

where

StridedArray{T,N} = Union{DenseArray{T,N}, StridedSubArray{T,N}, StridedReshapedArray{T,N}, StridedReinterpretArray{T,N}}

My understanding is that the proper definitions would be something like could be

function $(fname)(...::StridedArray) ...

where the following should be enough

_iscontiguous(::DenseArray) = true
_iscontiguous(::StridedReinterpretArray) = true
_iscontiguous(::StridedReshapedArray) = true
_iscontiguous(::StridedSubArray) = false #Not sure what a good fallback would be
_iscontiguous(::StridedSubVector) = stride(A,1) == 1 # This should at least catch all vector cases

or if we dont have to worry about new types being added to StridedArray

_iscontiguous(::StridedSubVector) = stride(A,1) == 1
_iscontiguous(::StridedSubArray) = false #Not sure what a good fallback would be
_iscontiguous(::StridedArray) = true # Catch all other cases

mfalt avatar Apr 06 '19 01:04 mfalt