parametrize Bidiagonal on wrapped vector type #22925

fredrikekre · 2017-07-23T16:18:11Z

Same as #22718 but for Bidiagonal.

tkelman · 2017-07-23T19:48:08Z

test/linalg/bidiag.jl

@@ -97,29 +97,30 @@ srand(1)
 end

 @testset "triu and tril" begin
+ bidiagcopy(dv, ev, uplo) = Bidiagonal(copy(dv), copy(ev), uplo)


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

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

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

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 · 2017-07-25T11:38:29Z

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 · 2017-07-29T00:01:38Z

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

tkelman · 2017-07-29T03:53:30Z

base/linalg/bidiag.jl

 end

-function Bidiagonal(dv::AbstractVector{Td}, ev::AbstractVector{Te}, uplo::Symbol) where {Td,Te}
- T = promote_type(Td,Te)
+function Bidiagonal(dv::AbstractVector{T}, ev::AbstractVector{T}, uplo::Symbol) where T
 Bidiagonal(convert(Vector{T}, dv), convert(Vector{T}, ev), uplo)


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

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.

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.

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.

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

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 · 2017-07-30T22:04:42Z

Alright, updated the PR.

tkelman · 2017-07-31T22:04:22Z

base/deprecated.jl

+# PR #22925
+# also uncomment constructor tests in test/linalg/bidiag.jl
+function Bidiagonal(dv::AbstractVector{T}, ev::AbstractVector{S}, uplo::Symbol) where {T,S}
+ depwarn(string("Bidiagonal(dv::AbstractVector{T}, ev::AbstractVector{S}, uplo::Symbol) ",


add a bullet for this deprecation in NEWS

Right, updated.

tkelman · 2017-07-31T22:05:55Z

base/linalg/bidiag.jl

 1 ⋅ ⋅ ⋅
 2 2 ⋅ ⋅
 ⋅ 3 3 ⋅
 ⋅ ⋅ 4 4
 ```
 """
-Bidiagonal(A::AbstractMatrix, uplo::Symbol) = Bidiagonal(diag(A), diag(A, uplo == :U ? 1 : -1), uplo)
+function Bidiagonal(A::AbstractMatrix, uplo::Symbol)
+ Bidiagonal(diag(A, 0), diag(A, uplo == :U ? 1 : -1), uplo)


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

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

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

fredrikekre · 2017-08-01T23:19:43Z

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

tkelman · 2017-08-02T04:35:22Z

NEWS.md

@@ -147,6 +147,9 @@ Library improvements
 allowing `Diagonal` and `Bidiagonal` matrices with arbitrary
 `AbstractVector`s ([#22718], [#22925]).

+ * `Bidiagonal` constructors that automatically converted the input vectors to
+ the same type are deprecated in favor of explicit conversion ([#22925]).


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

tkelman · 2017-08-02T04:37:41Z

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?

deprecate Bidiagonal constructor that automatically convert the vectors

fredrikekre · 2017-08-03T01:35:57Z

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

tkelman · 2017-08-03T01:47:49Z

I'm not seeing a docstring change?

fredrikekre · 2017-08-03T01:49:48Z

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

tkelman · 2017-08-03T02:06:24Z

no, I just missed that, probably good enough

nanosoldier · 2017-08-03T02:41:41Z

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

tkelman · 2017-08-03T23:16:46Z

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

fredrikekre · 2017-08-03T23:18:03Z

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

It happens from time to time

StefanKarpinski · 2017-08-03T23:29:34Z

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

amitmurthy · 2017-08-04T07:09:44Z

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 · 2017-08-04T11:43:11Z

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.

fredrikekre added kind:breaking This change will break code domain:linear algebra Linear algebra labels Jul 23, 2017

tkelman reviewed Jul 23, 2017

View reviewed changes

Sacha0 approved these changes Jul 25, 2017

View reviewed changes

fredrikekre force-pushed the fe/bidiagonal branch 2 times, most recently from 6177b3b to 1a13b38 Compare July 28, 2017 23:58

tkelman reviewed Jul 29, 2017

View reviewed changes

fredrikekre mentioned this pull request Jul 30, 2017

parametrize SymTridiagonal on the wrapped vector type #23035

Merged

fredrikekre force-pushed the fe/bidiagonal branch from 1a13b38 to b2d4167 Compare July 30, 2017 22:00

fredrikekre force-pushed the fe/bidiagonal branch from b2d4167 to ff3d5a0 Compare July 30, 2017 22:10

fredrikekre mentioned this pull request Jul 31, 2017

diag(sparse matrix) should be a sparse vector? #21064

Closed

fredrikekre force-pushed the fe/bidiagonal branch from ff3d5a0 to aa3ee45 Compare July 31, 2017 11:16

tkelman reviewed Jul 31, 2017

View reviewed changes

tkelman reviewed Aug 2, 2017

View reviewed changes

parametrize Bidiagonal on wrapped vector type

7993842

deprecate Bidiagonal constructor that automatically convert the vectors

fredrikekre force-pushed the fe/bidiagonal branch from 8c061c9 to 7993842 Compare August 3, 2017 01:33

tkelman merged commit 8832208 into JuliaLang:master Aug 3, 2017

fredrikekre deleted the fe/bidiagonal branch August 3, 2017 23:17

fredrikekre mentioned this pull request Aug 6, 2017

parametrize Tridiagonal on the wrapped vector type #23154

Merged

fredrikekre mentioned this pull request Aug 28, 2017

fix some convert methods that depwarned #23494

Merged

fredrikekre mentioned this pull request Oct 20, 2017

diag specializations for structured matrices yield dense vectors #24227

Closed

cormullion mentioned this pull request Mar 18, 2018

NEWS.md is getting a bit untidy #26508

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

parametrize Bidiagonal on wrapped vector type #22925

parametrize Bidiagonal on wrapped vector type #22925

fredrikekre commented Jul 23, 2017

tkelman Jul 23, 2017

fredrikekre Jul 23, 2017

tkelman Jul 23, 2017

fredrikekre Jul 23, 2017

Sacha0 left a comment

fredrikekre commented Jul 25, 2017

fredrikekre commented Jul 29, 2017

tkelman Jul 29, 2017

fredrikekre Jul 29, 2017

fredrikekre Jul 30, 2017

fredrikekre Jul 30, 2017

tkelman Jul 30, 2017

tkelman Jul 30, 2017 •

edited

Loading

fredrikekre commented Jul 30, 2017

tkelman Jul 31, 2017

fredrikekre Aug 3, 2017

tkelman Jul 31, 2017

fredrikekre Aug 1, 2017

tkelman Aug 1, 2017

fredrikekre commented Aug 1, 2017

tkelman Aug 2, 2017

tkelman commented Aug 2, 2017

fredrikekre commented Aug 3, 2017

tkelman commented Aug 3, 2017

fredrikekre commented Aug 3, 2017

tkelman commented Aug 3, 2017

nanosoldier commented Aug 3, 2017

tkelman commented Aug 3, 2017

fredrikekre commented Aug 3, 2017

StefanKarpinski commented Aug 3, 2017

amitmurthy commented Aug 4, 2017

StefanKarpinski commented Aug 4, 2017

parametrize Bidiagonal on wrapped vector type #22925

parametrize Bidiagonal on wrapped vector type #22925

Conversation

fredrikekre commented Jul 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Sacha0 left a comment

Choose a reason for hiding this comment

fredrikekre commented Jul 25, 2017

fredrikekre commented Jul 29, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman Jul 30, 2017 • edited Loading

Choose a reason for hiding this comment

fredrikekre commented Jul 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fredrikekre commented Aug 1, 2017

Choose a reason for hiding this comment

tkelman commented Aug 2, 2017

fredrikekre commented Aug 3, 2017

tkelman commented Aug 3, 2017

fredrikekre commented Aug 3, 2017

tkelman commented Aug 3, 2017

nanosoldier commented Aug 3, 2017

tkelman commented Aug 3, 2017

fredrikekre commented Aug 3, 2017

StefanKarpinski commented Aug 3, 2017

amitmurthy commented Aug 4, 2017

StefanKarpinski commented Aug 4, 2017

tkelman Jul 30, 2017 •

edited

Loading