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

Remove StridedArray restrictions on factorization objects. #620

Closed
dlfivefifty opened this issue Mar 18, 2019 · 5 comments · Fixed by JuliaLang/julia#47107
Closed

Remove StridedArray restrictions on factorization objects. #620

dlfivefifty opened this issue Mar 18, 2019 · 5 comments · Fixed by JuliaLang/julia#47107
Labels
arrays [a, r, r, a, y, s]

Comments

@dlfivefifty
Copy link
Contributor

I'm going through the QR, Cholesky, etc. factorization codes as I re-write the factorizations in BandedMatrices.jl (and elsewhere) to be consistent with Base, and there are a large amount of unnecessary restrictions to StridedArrays. This is particularly silly as many of the factorizations have generic Julia fallbacks which do not use strided-ness at all.

I can do a PR when I get some time.

@dlfivefifty
Copy link
Contributor Author

And we also need to add restrictions to StridedArray where LAPack is called, here is an example:

https://github.com/JuliaLang/julia/blob/8c0b550b61bcb196e1c8a9b03b38f0e6eb8a9dd7/stdlib/LinearAlgebra/src/cholesky.jl#L398

@mbauman
Copy link
Member

mbauman commented Mar 18, 2019

You may find JuliaLang/julia#31149 helpful as you go through these — I use it semi-regularly when dealing with method lists that are ridiculous. I've not merged it since nobody has reviewed or voiced their support for it.

@mbauman mbauman added the arrays [a, r, r, a, y, s] label Mar 18, 2019
@andreasnoack
Copy link
Member

This is particularly silly as many of the factorizations have generic Julia fallbacks which do not use strided-ness at all.

I completely agree for some of the "outer" functions which only converts to the right element type but I have more mixed feelings for the actual implementations. While the generic implementations don't use the stridedness of StridedArrays explicitly, restricting to StridedArray ensures the elementwise access to the array is not prohibitively slow. Hitting a generic dense linear algebra routine with a sparse or distributed arrays is quite frustrating.

@dlfivefifty
Copy link
Contributor Author

We could overload for AbstractSparseArray to throw an error

@andreasnoack
Copy link
Member

We could overload for AbstractSparseArray to throw an error

Something like might end up as the solution. GPUArrays and now also DistributedArrays have a flag to disallow scalar getindex to make it easier to detect when you hit AbstractArray implementations that fetch elements one-by-one.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrays [a, r, r, a, y, s]
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants