parametrize Bidiagonal on wrapped vector type

[fredrikekre]

Same as #22718 but for Bidiagonal.

[tkelman]

hm, so was this previously writing zeros through to these?

[fredrikekre]

No but previously the constructor always made a copy of the vectors, ref e0d75d7#commitcomment-22968553

[tkelman]

were Tridiagonal or SymTridiagonal behaving like that too, or was Bidiagonal the only case of it?

[fredrikekre]

e0d75d7 only changed the behaviour of Bidiagonal. The other ones still have constructors that don't copy: Diagonal, Tridiagonal and SymTridiagonal.

[Sacha0]

generally lgtm! :)

[fredrikekre]

Restarting Travis, I saved the logs if anyone is interested, but:
Timeout on i686.
The following on x86_64:

The command "du -sk /tmp/julia/*" exited with 1.

0.02s$ if [ `uname` = "Darwin" ]; then for name in suitesparseconfig spqr umfpack colamd cholmod amd suitesparse_wrapper; do install -pm755 usr/lib/lib${name}*.dylib* /tmp/julia/lib/julia/; done; fi 

and some (spurious?) distributed failure on macOS.

[fredrikekre]

Added some more constructors which promotes more properly, and tests.

[tkelman]

isn't this conversion to Vector what you're trying to make unnecessary?

[fredrikekre]

This is just a fallback for cases like Bidiagonal(::Vector{T}, ::GenericVector{T}), but perhaps the constructor on line 62 should be generalized to promote to the same container type as well, and not just promote the element type? That would make this constructor obsolete.
Currently the promotion is as follows:

Bidiagonal(::Vector{T}, ::GenericVector{S}) ->
Bidiagonal(::Vector{R}, ::GenericVector{R}) ->
Bidiagonal(::Vector{R}, ::Vector{R})

The problem with generalizing the method above was this:

julia> A = rand(3); B = GenericArray(rand(3)); a, b = promote(A,B);

julia> typeof(a)
Array{Float64,1}

julia> typeof(b)
Base.Test.GenericArray{Float64,1}

i.e. they don't promote to the same container type. Hence the current scheme.

[fredrikekre]

Thinking about this a bit more, perhaps we should not promote anything in the constructor here, and instead let the user promote to appropriate types. In that case this:

function Bidiagonal(dv::V, ev::V, uplo::Symbol) where {T,V<:AbstractVector{T}}
    Bidiagonal{T}(dv, ev, char_uplo(uplo))
end

would be the only constructor. The good thing about this is that we don't try to "figure out" what the user wants; instead the user have to be explicit about this. The other good thing with this method is that the vectors in the Bidiagonal will always alias the vectors sent to the constructor. This would then be consistent with the new Diagonal type from #22718 . As this PR currently stands, the vectors will sometimes alias and sometimes not.

[fredrikekre]

We would also have to deprecate methods that did convert, but I still like this better. It is probably not very often you supply two different vector types anyway; and it would be consistent that the vectors always alias, so I will update the PR like that later, unless other people think that’s a bad idea.

[tkelman]

or promote in a way consistent with what concatenation would do, since this is sort of like a diagonal concatenation

[tkelman]

the aliasing predictability benefits of not promoting at all and making the user pick could also have advantages though - as long as the old methods were deprecated, and the restriction is documented, I think that would also be fine and not trip up too many people

[fredrikekre]

Alright, updated the PR.

[tkelman]

add a bullet for this deprecation in NEWS

[fredrikekre]

Right, updated.

[tkelman]

I didn't actually realize diag(A, k) would already give a SparseVector

[fredrikekre]

Yeah, as long as the two arg form is inferable this should be "more correct" since diag(A) in some cases give something else.

[tkelman]

which isn't very consistent with it acting like a default argument, but that's a discussion for a different issue

[fredrikekre]

Added NEWS for the deprecations. Would like to merge this first and then fix up #23035 afterwards.

[tkelman]

I was thinking this would go in the Deprecated or removed section since the library functionality part of the change is mentioned earlier

[tkelman]

I'd also add to the docstring what restrictions on the two inputs there are - in Bidiagonal(dv, ev, uplo::Symbol) is it now that dv and ev could have different element types but must otherwise be the same subtype of AbstractVector, or do they have to be the same type including eltype?

[fredrikekre]

Updated the docstring for the constructor. There are some Bidiagonal benchmarks in BaseBenchmarks.jl, so before merging:
@nanosoldier runbenchmarks("linalg", vs = ":master")

[tkelman]

I'm not seeing a docstring change?

[fredrikekre]

I only added the constraint to the signature, you want me to mention it in the text too?

[tkelman]

no, I just missed that, probably good enough

[nanosoldier]

Your benchmark job has completed - no performance regressions were detected. A full report can be found here. cc @ararslan

[tkelman]

win32 appveyor failure in distributed is a bit alarming, but highly doubt it's related

[fredrikekre]

win32 appveyor failure in distributed is a bit alarming, but highly doubt it's related

It happens from time to time

[StefanKarpinski]

Good lord 😠. Getting our CI rock solid has got to be a top priority post 1.0 if not sooner.

[amitmurthy]

The randomly occurring distributed failure is a segfault . Stack shows gf.c, but then the corruption could have happened elsewhere too.

The distributed module does not have any ccalls - consequently I don't know how to track it down especially since it does not occur locally.

Long term CI plans should include a valgrind / memcheck run if possible.

[StefanKarpinski]

I'm not complaining about this particular issue really, just the general flakiness of our CI lately. A bunch of it is that Travis keeps giving us less resources so now many runs just time out.