Add xggsvd3 wrappers. #14444

andreasnoack · 2015-12-19T01:21:08Z

The subroutines replace xggsvd in LAPACK 3.6.0 so calls are made conditionally on the existence of the symbols.

Update: I've also changed the blasfunc function to a macro because the function was awkward to use without a loop because of the @eval block. E.g. the new LAPACK.laver() definition.

cc: @yuyichao

yuyichao · 2015-12-19T01:31:16Z

base/linalg/svd.jl

@@ -77,7 +77,12 @@ end
 GeneralizedSVD{T}(U::AbstractMatrix{T}, V::AbstractMatrix{T}, Q::AbstractMatrix{T}, a::Vector, b::Vector, k::Int, l::Int, R::AbstractMatrix{T}) = GeneralizedSVD{T,typeof(U)}(U, V, Q, a, b, k, l, R)

 function svdfact!{T<:BlasFloat}(A::StridedMatrix{T}, B::StridedMatrix{T})
-    U, V, Q, a, b, k, l, R = LAPACK.ggsvd!('U', 'V', 'Q', A, B)
+    # xggsvd3 replaced xggsvd in LAPACK 3.6.0


Can we initialize this in __init__ or use cglobal to avoid a dlsym on every function call?

That would be cleaner even though this function is so expensive that I don't think it matters much. I'll update the PR.

I have now defined a constant with the LAPACK version number instead.

yuyichao · 2015-12-19T04:09:04Z

base/linalg/lapack.jl


 import ..LinAlg: BlasFloat, Char, BlasInt, LAPACKException,
    DimensionMismatch, SingularException, PosDefException, chkstride1, chksquare

+function __init__()
+    const global VERSION = VersionNumber(laver()...)


This is actually bad .... For one thing, pre-compilation won't treat the version number as a constant.

Can you explain a bit more? When is the constant set? It should correspond to the version actually linked when Julia is launched. What is the right solution?

Ref #12010 and #13817

tkelman · 2015-12-19T10:50:18Z

base/linalg/lapack.jl

+and `V'*B*Q = D2*R`. `D1` has `alpha` on its diagonal and `D2` has `beta` on its
+diagonal. If `jobu = U`, the orthogonal/unitary matrix `U` is computed. If
+`jobv = V` the orthogonal/unitary matrix `V` is computed. If `jobq = Q`,
+the orthogonal/unitary matrix `Q` is computed. If `job{u,v,q} = N`, that


curly braces are a little confusing here since they mean type parameters in julia code

tkelman · 2015-12-19T10:54:47Z

What error do you get in the case that your lapack is too new or too old and you try to call the LAPACK. wrapper function directly? If these are interchangeable and we won't have both available at the same time, should we put them in the same Julia wrapper method?

yuyichao · 2015-12-19T19:27:25Z

base/linalg/lapack.jl

+        (Ptr{BlasInt}, Ptr{BlasInt}, Ptr{BlasInt}),
+        major, minor, patch)
+    return major[1], minor[1], patch[1]
+end


Although this function is only used during initialization time, maybe it's better to just return a VersionNumber from this function? Or is a tuple easier in some cases? Also I think allocating a Ref{BlasInt}() is faster than BlasInt[0].

The latest version uses Refs, but I've kept the tuple return because, in general, the return from the LAPACK is similar to what is returned from the subroutines. Sometimes we make some postprocessing in the wrappers, but most often this is done down the pipeline and I don't think we return "special" Julia types from any of the LAPACK wrappers.

Fair enough.

I have no idea about the function you are wrapping but it otherwise LGTM. =)

andreasnoack · 2015-12-23T13:53:46Z

@tkelman The old unblocked and the new blocked API are very similar. However, they have added a build flag for deprecated functions which would give both versions and I'm not sure how MKL and Accelerate will handle a deprecation like this. It is also slightly clearner to have two functions instead of one with more branching but, of course, with the cost of some code duplication.

tkelman · 2015-12-23T14:02:31Z

If they're completely redundant then I disagree that two functions is cleaner than one, but it sounds like under some configurations they might not be.

Don't use job{u,v,q} in the docstring, that's too easily confusable with Julia type parameter syntax which isn't what you mean there.

@eval

…so calls are made conditionally on the existence of the symbols by defining a constant with the LAPACK version number. Make blasfunc a macro such that it can be called without @eval $name.

andreasnoack · 2015-12-23T15:26:52Z

Don't use job{u,v,q} in the docstring, that's too easily confusable with Julia type parameter syntax which isn't what you mean there.

I forgot your comment to the documentation. The entry was copied verbatim from ggsvd!, but I have now changed both versions.

Add xggsvd3 wrappers.

tkelman · 2015-12-28T14:46:02Z

base/linalg/lapack.jl

+    major = BlasInt[0]
+    minor = BlasInt[0]
+    patch = BlasInt[0]
+    ccall((@blasfunc(ilaver_), liblapack), Void,


Should this be :ilaver_ ? It's inside a macro call so apparently works anyway, but looks a little strange.

I think a macro can handle :ilaver_ and ilaver_ equally well (probably even easier without the quote) so may as well save some typing?

Just looks strange to be using what looks like, but isn't, a defined Julia object.

julia> macro whatisthis(thing) return typeof(thing) end julia> @whatisthis(:ilaver_) Expr julia> @whatisthis(ilaver_) Symbol julia> for s in (:ilaver_,) @eval println(@whatisthis($s)) end Symbol

I was also puzzled by this. The macro could be modified to work for Expr as well.

If we can get away without the $ on the @eval version that would seem cleaner to me

andreasnoack force-pushed the anj/ggsvd3 branch from 311d492 to 1a5e15d Compare December 19, 2015 01:21

yuyichao reviewed Dec 19, 2015
View reviewed changes

yuyichao added the linear algebra Linear algebra label Dec 19, 2015

andreasnoack force-pushed the anj/ggsvd3 branch from 1a5e15d to c1653fe Compare December 19, 2015 03:52

yuyichao reviewed Dec 19, 2015
View reviewed changes

andreasnoack force-pushed the anj/ggsvd3 branch from c1653fe to 2bf20c1 Compare December 19, 2015 05:03

tkelman reviewed Dec 19, 2015
View reviewed changes

yuyichao reviewed Dec 19, 2015
View reviewed changes

andreasnoack force-pushed the anj/ggsvd3 branch from 2bf20c1 to efd9a60 Compare December 23, 2015 13:27

andreasnoack force-pushed the anj/ggsvd3 branch from efd9a60 to 30fc2ee Compare December 23, 2015 14:53

Add xggsvd3 wrappers. The subroutines replace xggsvd in LAPACK 3.6.0 …

15c70cb

…so calls are made conditionally on the existence of the symbols by defining a constant with the LAPACK version number. Make blasfunc a macro such that it can be called without @eval $name.

andreasnoack force-pushed the anj/ggsvd3 branch from 30fc2ee to 15c70cb Compare December 23, 2015 15:26

andreasnoack added a commit that referenced this pull request Dec 28, 2015

Merge pull request #14444 from JuliaLang/anj/ggsvd3

57eb9c1

Add xggsvd3 wrappers.

andreasnoack merged commit 57eb9c1 into master Dec 28, 2015

andreasnoack deleted the anj/ggsvd3 branch December 28, 2015 13:39

tkelman reviewed Dec 28, 2015
View reviewed changes

rgommers mentioned this pull request Jan 16, 2016

Deprecated routines in Netlib LAPACK >3.5.0 scipy/scipy#5266

Closed

yuyichao mentioned this pull request Apr 1, 2016

Test error on 0.4 release branch #15733

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add xggsvd3 wrappers. #14444

Add xggsvd3 wrappers. #14444

andreasnoack commented Dec 19, 2015

yuyichao Dec 19, 2015

andreasnoack Dec 19, 2015

andreasnoack Dec 19, 2015

yuyichao Dec 19, 2015

andreasnoack Dec 19, 2015

yuyichao Dec 19, 2015

tkelman Dec 19, 2015

tkelman commented Dec 19, 2015

yuyichao Dec 19, 2015

andreasnoack Dec 23, 2015

yuyichao Dec 23, 2015

andreasnoack commented Dec 23, 2015

tkelman commented Dec 23, 2015

andreasnoack commented Dec 23, 2015

tkelman Dec 28, 2015

yuyichao Dec 28, 2015

tkelman Dec 28, 2015

andreasnoack Dec 28, 2015

tkelman Dec 28, 2015

Add xggsvd3 wrappers. #14444

Add xggsvd3 wrappers. #14444

Conversation

andreasnoack commented Dec 19, 2015

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tkelman commented Dec 19, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andreasnoack commented Dec 23, 2015

tkelman commented Dec 23, 2015

andreasnoack commented Dec 23, 2015

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