Skip to content
This repository has been archived by the owner on Apr 9, 2020. It is now read-only.

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

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

mfalt
Copy link

@mfalt mfalt commented Apr 4, 2019

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
Copy link
Author

mfalt commented Apr 6, 2019

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
JuliaLang/julia@9d5a05e#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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant