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

Tighten StridedVector -> Vector in a few spots. Fix #13737. #16284

Merged
merged 1 commit into from
May 16, 2016

Conversation

kshyatt
Copy link
Contributor

@kshyatt kshyatt commented May 9, 2016

The tau and select vectors in the LAPACK API don't come with
a stride option. I've tightened our requirement to straight
Vector as discussed at JuliaLang/LinearAlgebra.jl#272.

@kshyatt kshyatt added the linear algebra Linear algebra label May 9, 2016
@andreasnoack
Copy link
Member

I think that this is fine and nobody will get hurt but I think another solution could be just to check that the stride is one.

@kshyatt
Copy link
Contributor Author

kshyatt commented May 10, 2016

@andreasnoack it seems dishonest to me to say that StridedVector is an allowed input if only one stride is actually acceptable.

@andreasnoack
Copy link
Member

There can be all kind of restriction to the strides (which there already are in the BLAS/LAPACK wrappers) and if someone would like to use part of an existing allocation for one of these vector then a SubArray would be their only option.

@JaredCrean2
Copy link
Contributor

JaredCrean2 commented May 10, 2016

It seems like this is a more general problem of what the right abstraction around a contiguous array view is. When dealing with C code, having vector defined as typealias Vector Array{T, 1} is definitely too tight, and StridedArray is too loose. Maybe this is crazy, inserting a new ContiguousArray in the type hierarchy above Array would solve this problem.

Now check that the stride is 1, following what we already did
with the matrix arguments.
@kshyatt kshyatt force-pushed the ksh/lapackstride branch from cf18ba0 to 379c649 Compare May 11, 2016 22:50
@kshyatt
Copy link
Contributor Author

kshyatt commented May 15, 2016

Bumping this.

@JaredCrean2
Copy link
Contributor

The more I think about this issue, the more I am convinced solve it requires some kind of interface specification that says "this array can be passed to C". Whether that is an abstract type or a trait-ish thing I'm not sure.

In either case, the important behavior is that pointer(A) returns a pointer that can be used to access the entire array A.

@andreasnoack
Copy link
Member

Looks good to me. We might handle this differently in the future but this is definitely an improvement.

@JaredCrean2 See #15308.

@andreasnoack andreasnoack merged commit 5896d69 into master May 16, 2016
@kshyatt kshyatt deleted the ksh/lapackstride branch May 16, 2016 00:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linear algebra Linear algebra
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants