Add tests for SubArrays in test/linalg/bunchkaufman.jl

[pkofod]

• Added tests for linalg/bunchkaufman.jl. Found tiny dispatch bug, as factorize didn't dispatch properly for SubArrays (changed Matrix -> StridedMatrix).
• Moved two matrix calculations in svd.jl.
• Added tests for givens.jl. Again, at loop over Array and SubArray, here for the matrix A, and vector x. SubArrays seem to pass just fine.

[andreasnoack]

It might be better to make individual pull requests for each of the files or maybe a couple and use #14732 to track the progress. It's easier to review smaller PRs and since most of the changes probably won't depend on each other it shouldn't cause issues to split them up.

[pkofod]

I agree, edited to reflect your comment. It should be good to go, let's see if Travis agrees.

[pkofod]

Doing the bunchkaufman.jl tests, I realized I had placed the asym and apd matrices in such a way in svd.jl, that they were not SubArrays, which made coverage worse than I thought. It didn't matter when I moved them in svd.jl (most recent commit), but it was what made me catch the factorize bug in bunchkaufman.jl.

[pkofod]

Added tests for givens.jl. Again, at loop over Array and SubArray, here for the matrix A, and vector x. SubArrays seem to pass just fine.

[tkelman]

should this be sub(b, ?

[pkofod]

Of course! Will fix

[pkofod]

Fixing it, such that a-> b makes

@test_approx_eq_eps asym * (bc1\b) b 1000ε

fail with

ERROR: LoadError: MethodError: no method matching reinterpret(::Type{Float32}, ::SubArray{Complex{Float32},2,Array{Complex{Float32},2},Tuple{UnitRange{Int64},UnitRange{Int64}},false}, ::Tuple{Int64,Int64})

Will try to fix it.

[pkofod]

Adding the SubArray tests caused https://github.com/JuliaLang/julia/pull/15354/files#diff-be0f75db178315152e44756ee149ffd8R58 to fail for eltypeb==Complex64 and typeb=="SubArray" together. I fixed this at the \ level, by calling parent on B. Thoughts on this?

[andreasnoack]

Using parent seems correct. LGTM. @tkelman do you have further comments?

[tkelman]

Just one - given #15367, should StridedMatrix continue to be used rather than AbstractMatrix ?

[pkofod]

I can change it if it's appropriate, and if there's no reason to restrict it to StridedMatrix, I assume that it is.

Is there an easy way to go back to the first commit and change it? I guess I would branch, reset to the commit, make the change, and then merge with my original branch. Is that the way to go?

[KristofferC]

You can do git rebase -i HEAD~3 where the 3 is the number of commits you want to go back and then pick edit for the first commit and do your changes.

See for example: http://stackoverflow.com/questions/1186535/how-to-modify-a-specified-commit-in-git

[pkofod]

Perfect, thank you!

Edit: I am just wondering: since all of the other functions around it has StridedMatrix maybe this is a more general discussion, or?

[andreasnoack]

I'm not sure if these work well beyond StridedMatrix. Some of the methods would probably fail for some DArrays or SparseMatrixCSCs. That might just suggest that some other methods should be fixed to work correctly for those matrix types, but I think we should merge this as it is and wait for a more specific need for AbstractMatrix.

[timholy]

The use of parent is problematic because the parent may not be the same size as the subarray:

julia> A = rand(6,5); A = A'*A
5x5 Array{Float64,2}:
 2.79981  1.44259  1.9957    2.33348  1.43793 
 1.44259  1.50449  1.16219   1.20452  1.22158 
 1.9957   1.16219  1.93965   1.61173  0.964557
 2.33348  1.20452  1.61173   1.9936   1.11126 
 1.43793  1.22158  0.964557  1.11126  1.66233 

julia> F = cholfact(A);

julia> v6 = rand(Complex128, 6)
6-element Array{Complex{Float64},1}:
 0.485376+0.020852im 
 0.855035+0.32091im  
 0.152016+0.935278im 
  0.78076+0.0182181im
 0.954256+0.378917im 
 0.982591+0.995221im 

julia> v5 = sub(v6, 1:5)
5-element SubArray{Complex{Float64},1,Array{Complex{Float64},1},Tuple{UnitRange{Int64}},true}:
 0.485376+0.020852im 
 0.855035+0.32091im  
 0.152016+0.935278im 
  0.78076+0.0182181im
 0.954256+0.378917im 

julia> F\v5
ERROR: DimensionMismatch("new dimensions (2,5) must be consistent with array size 12")
 in reinterpret(::Type{Float64}, ::Array{Complex{Float64},1}, ::Tuple{Int64,Int64}) at ./array.jl:94
 in \(::Base.LinAlg.Cholesky{Float64,Array{Float64,2}}, ::SubArray{Complex{Float64},1,Array{Complex{Float64},1},Tuple{UnitRange{Int64}},true}) at ./linalg/factorization.jl:27
 in eval(::Module, ::Any) at ./boot.jl:243

[andreasnoack]

Maybe the original version was correct if we can define reinterepret for SubArray.

[pkofod]

Well that is obviously not intended. To to be sure, is this something you're working on @timholy or were you just pointing it out?

[timholy]

Haven't started trying to fix it yet. It was going to get in my way (so I might have fixed it quickly), but then I had to take a step back and think some more.

[timholy]

Oh, and @andreasnoack: since reinterpret can reshape an array (changing the dimensionality), and reshape doesn't currently compose with SubArray, I suspect this would be a hard path. (But maybe someday.)

[pkofod]

Okay, if I can help let me know.

[timholy]

Oh, I meant "think some more" about my BIG SCARY problem, not this specific issue. Sorry for the confusion 😄.

[pkofod]

Oh, okay! So the bug is still there? I'm confused, sorry :)

[timholy]

Yes, AFAIK the bug is still there, and I'm not working on it.