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

WIP:New design of linalg (factorizations) #2308

Merged
merged 45 commits into from
Mar 16, 2013
Merged

WIP:New design of linalg (factorizations) #2308

merged 45 commits into from
Mar 16, 2013

Conversation

andreasnoack
Copy link
Member

Based on the discussion in #2212 I have rewritten some of the linalg functionality. Please share your thoughts. The main points of the design are

Matlab style methods are retained for now

Removal of factors methods and introduction of ref methods for factorizations object. Symbols are used for referencing. E.g.

julia> A=randn(5,3)
5x3 Float64 Array:
 -1.84808   -1.56926   -0.394121 
  2.5735     1.11443    0.0477358
  0.751048   0.920844   0.756794 
 -0.987947  -1.39258   -1.59187  
  0.684266   1.28512   -1.37579  

julia> Aqr=QRDense(copy(A));

julia> Aqr[:Q]*Aqr[:R]
5x3 Float64 Array:
 -1.84808   -1.56926   -0.394121 
  2.5735     1.11443    0.0477358
  0.751048   0.920844   0.756794 
 -0.987947  -1.39258   -1.59187  
  0.684266   1.28512   -1.37579  

Factorizations work similarly to the matrix they were constructed from. No magic.

New QRDensQ and HessenbergQ types which are subtypes of AbstractMatrix can be used for multiplication.

julia> typeof(Aqr[:Q])
QRDenseQ{Float64}

For types with pivoting you can get either a permutation vector of matrix

julia> A=randn(5,5)
5x5 Float64 Array:
 0.466243   1.15806    1.06016     0.479681  -0.0639797
 0.603621  -0.94684   -0.0267892   0.173938  -0.458464 
 0.87668   -2.22612    0.23515     2.37495    0.0723865
 0.849353   0.180417   1.08095    -0.45903   -0.993289 
 1.17308   -0.644301   0.202831    1.50354   -0.065989 

julia> LU=LUDense(copy(A));

julia> LU[:P]'LU[:L]*LU[:U]
5x5 Float64 Array:
 0.466243   1.15806    1.06016     0.479681  -0.0639797
 0.603621  -0.94684   -0.0267892   0.173938  -0.458464 
 0.87668   -2.22612    0.23515     2.37495    0.0723865
 0.849353   0.180417   1.08095    -0.45903   -0.993289 
 1.17308   -0.644301   0.202831    1.50354   -0.065989 

julia> LU[:L][invperm(LU[:p]),:]*LU[:U]
5x5 Float64 Array:
 0.466243   1.15806    1.06016     0.479681  -0.0639797
 0.603621  -0.94684   -0.0267892   0.173938  -0.458464 
 0.87668   -2.22612    0.23515     2.37495    0.0723865
 0.849353   0.180417   1.08095    -0.45903   -0.993289 
 1.17308   -0.644301   0.202831    1.50354   -0.065989 

Removal of all fact methods. Factorizations are constructed from constructors. The fact methods were kind of constructors so I think it is more clear just to use e.g. QRDense instead of qrfact.

New Triangular and Hermitian types for dispatch. This eases the choice of algorithm and simplifies some of the functions for general matrices. E.g.

julia> B=randn(5,2)
i5x2 Float64 Array:
 -1.67605    0.0792652
 -2.1588     0.0286816
 -0.252934  -0.082285 
  0.777874   1.55441  
 -1.9374     0.949172 

julia> issym(B/(B'B)*B')
false

julia> issym(Hermitian(B/(B'B)*B'))
true

julia> eigvals(B/(B'B)*B') # non symmetric solver
5-element Float64 Array:
 -1.11022e-16
  1.0        
  0.0        
  1.0        
 -1.74743e-17

julia> eigvals(Hermitian(B/(B'B)*B')) # symmetric solver
5-element Float64 Array:
 -1.96396e-16
 -1.24191e-16
 -7.64151e-17
  1.0        
  1.0        

New symmetric tridiagonal eigen solvers and methods for subsets of eigenvalues

julia> eigvals(Hermitian(B/(B'B)*B'), 4, 5)
2-element Float64 Array:
 1.0
 1.0

julia> eigvals(Hermitian(B/(B'B)*B'), 0.9, 1.1)
2-element Float64 Array:
 1.0
 1.0

Removal Bool argument in eig. Just use eigvals.

@dmbates
Copy link
Member

dmbates commented Feb 15, 2013

For a QR decomposition is there a way of multiplying by the full Q or Q' without forming the matrix Q? For example, one of the most stable ways of forming residuals is Q * diagmm(vcat(zeros(k), ones(n-k)), Q'y) where k is the column rank of the matrix that was decomposed (X), n is size(X,1), and n > k. Because Q is implicitly n by n and multiplications by Q or Q' are applications of k Householder transformations a product like Q'y can be quickly and accurately evaluated even when n >> k. What I am trying to get at is that Q is sometimes used as an n by n (implicit) orthogonal matrix and not just the first k columns.

Of course these operations can be done effectively by direct calls to functions in lapack.jl .

@andreasnoack
Copy link
Member Author

Right now I have defined the multiplication methods for QRDenseQ such that for Q you can multiply with either the thin Q or the full Q. E.g.

julia> A=randn(5,2);

julia> Aqr=QRDense(A);

julia> Aqr[:Q]*randn(2,2)
5x2 Float64 Array:
  0.476723   0.727353
 -0.320566   0.429844
 -0.761412  -0.148256
  0.144157   0.538303
  0.781046  -0.176017

julia> Aqr[:Q]*randn(5,2)
5x2 Float64 Array:
  0.727572   1.40746  
 -0.260413   0.016806 
  0.743548   0.0498523
  0.697383  -0.635056 
 -0.439073  -1.394    

julia> Aqr[:Q]*randn(4,2)
ERROR: DimensionMismatch("")
 in * at linalg_dense.jl:690

For Q', it is assumed that you multiply with the full Q. These methods all use the LAPACK interface for multiplication with Q so no intermediate nxn arrays.

@dmbates
Copy link
Member

dmbates commented Feb 15, 2013

Thanks for the info and good job on this.

@nolta
Copy link
Member

nolta commented Feb 15, 2013

Nice, i like it. A few comments:

  • CholeskyDense(a)[:L] should return the transpose, not fail.
  • Probably better if CholeskyDense(a)[:UL] returned Triangular.
  • I'm not keen on the ref(::Factorization, ::Integer) methods. Can we get rid of them?
  • Could we rename QRDense -> QRFactorization, etc.?

@mlubin
Copy link
Member

mlubin commented Feb 16, 2013

Looks pretty good. Though why should we have to type CholeskyDense or CholeskyFactorization when the matlab versions (which we're trying to replace) get to have much shorter names? I'd rather have cholfact etc. Also cholfact could dispatch on the type of the matrix (dense/sparse). This is better than having to manually switch between CholeskyDense and CholeskySparse.

@toivoh
Copy link
Contributor

toivoh commented Feb 16, 2013

+1

@andreasnoack
Copy link
Member Author

@nolta I have changed ref for Cholesky such that the transpose is returned. Even though it is tempting to let CholeskyDense[:U] return Triangular I am not sure if it is the right thing to do. Right now it is mainly thought of as a way of controlling dispatching to the best possible LAPACK solver. There are no multiplication methods for Triangular and even though they would be easy to implement my experiments with symmetric matrix types showed that for multiplication, the general gemm was faster than the specialized functions in BLAS. I have removed the integer indexing. I don't like them either. Also @mlubin, I don't have strong feelings about names. They are easily changed so I'll let them be for now and see how the discussion ends.

@nolta
Copy link
Member

nolta commented Feb 17, 2013

Thanks @andreasnoackjensen. CholeskyDense[:U] returning a plain matrix is fine w/ me, i was thinking more that CholeskyDense[:UL] might want to return Triangular, since it's returning an "upper or lower" matrix.

@blakejohnson
Copy link
Contributor

I really like the new Triangular and Hermitian types for dispatch. This will allow us to specialize expm to use eigenvalue decomposition on Hermitian matrices, which will give a nice speed boost.

@andreasnoack
Copy link
Member Author

Please try this out and give some feedback about how it feels. It follows @ViralBShah's proposal of removing the MATLAB way of returning factorizations, but I hope it is more acceptable now that you can write chol(A)[:U]. This also makes it explicit whether the Cholesky is upper or lower.

I have changed the QR to use a new and faster subroutine from LAPACK, but the Julia user should see no changes except for speed gain. I have also added types and methods for rectangular full packed format which is a fast way of packing triangular and hermitian matrices. So if you have a least squares problem with a very large number of variables you can try chol(Ac_mul_A_RFP(A))\(A'B).

@ViralBShah
Copy link
Member

Should we compute the full Q in a QR factorization? May not be necessary, and it could be computed only when the Q is extracted. Also, I get undefs when trying to extract the Q.

julia> o = qr(rand(10,10))
QRDense{Float64}(10x10 Float64 Array:
 -1.94156    -1.23391    -1.60303     …  -1.68783    -1.83931   -1.00796  
  0.303564   -1.30836    -0.746558       -0.261806   -0.870288  -0.326301 
  0.25203     0.0529295  -1.0753         -0.361412   -0.706496  -0.803684 
  0.416338   -0.483915   -0.393628       -0.686878    0.142337   0.309002 
  0.0765375   0.129018    0.525529        0.153227   -0.284943  -0.150702 
  0.403948   -0.426361   -0.00813757  …   0.0490808  -0.291495   0.0961825
  0.163452    0.36595    -0.0702291      -0.443928   -0.348749   0.338881 
  0.433517   -0.127107   -0.133164        0.564233    0.254275   0.43618  
  0.122613    0.0649193   0.474859       -0.38811    -0.135627   0.12149  
  0.138093    0.235744    0.0914334       0.073302   -0.691776   0.128169 ,10x10 Float64 Array:
 1.14495       0.00235837    …  -1.06653       -0.671251      0.0
 2.16845e-314  1.21561          -0.137911       0.424522      0.0
 2.16845e-314  2.16845e-314      1.4326         0.746187      0.0
 2.16845e-314  2.16845e-314      0.910511       1.06865       0.0
 2.16845e-314  2.16845e-314     -0.363328      -0.818693      0.0
 2.16845e-314  2.16845e-314  …  -0.273501      -0.598814      0.0
 2.16845e-314  2.16845e-314      0.986161       0.394205      0.0
 2.16845e-314  2.16845e-314      1.7301         1.02695       0.0
 2.16845e-314  2.16845e-314      2.16845e-314   1.35267       0.0
 2.16845e-314  2.16845e-314      2.16845e-314   2.16845e-314  0.0)

julia> o[:Q]
10x10 Float64 QRDenseQ:
 #undef  #undef  #undef  #undef  #undef  …  #undef  #undef  #undef  #undef
 #undef  #undef  #undef  #undef  #undef     #undef  #undef  #undef  #undef
 #undef  #undef  #undef  #undef  #undef     #undef  #undef  #undef  #undef
 #undef  #undef  #undef  #undef  #undef     #undef  #undef  #undef  #undef
 #undef  #undef  #undef  #undef  #undef     #undef  #undef  #undef  #undef
 #undef  #undef  #undef  #undef  #undef  …  #undef  #undef  #undef  #undef
 #undef  #undef  #undef  #undef  #undef     #undef  #undef  #undef  #undef
 #undef  #undef  #undef  #undef  #undef     #undef  #undef  #undef  #undef
 #undef  #undef  #undef  #undef  #undef     #undef  #undef  #undef  #undef
 #undef  #undef  #undef  #undef  #undef     #undef  #undef  #undef  #undef

@ViralBShah
Copy link
Member

Cc: @alanedelman

@andreasnoack
Copy link
Member Author

@ViralBShah After the last update, qr computes the factorization by xgeqrt3 and hence what it actually stored in the factorization is only understandable for LAPACK. Hence the full Q is not computed in the factorization. Also, the reason you get undef#s is that I haven't defined ref for the QRDenseQ because I don't think it should be indexed and show for AbstractMatrix cannot print the values without ref. In order to get a real Q matrix you must write full(qr(A)[:Q], false) but that shouldn't be necessary as * methods are defined for QRDenseQ. An example

julia> A=randn(5,2)
5x2 Float64 Array:
 -1.04258      0.254472
  0.201797    -0.808045
 -0.889902     0.978295
  0.428517    -1.46212 
 -0.00136611  -0.264258

julia> Aqr=qr(A);

julia> Aqr[:Q]*Aqr[:R]
5x2 Float64 Array:
 -1.04258      0.254472
  0.201797    -0.808045
 -0.889902     0.978295
  0.428517    -1.46212 
 -0.00136611  -0.264258

@ViralBShah
Copy link
Member

Symmetric does not work, and Hermitian gives:

julia> a = rand(10,10); Symmetric(a'*a)
ERROR: Symmetric not defined

julia> a = rand(4,4); Hermitian(a'*a)
4x4 Float64 Hermitian:
 #undef  #undef  #undef  #undef
 #undef  #undef  #undef  #undef
 #undef  #undef  #undef  #undef
 #undef  #undef  #undef  #undef

@andreasnoack
Copy link
Member Author

For now, Hemitian covers Symmetric for real types, but it can easily be added. It is just a copy of the Hemitian code and a change of name. The undef is the same as before and is really just a matter of writing a show method. An easy fix would be to write ref(A::Hermitian, args...)=ref(full(A.S), args...) but you shouldn't start doing heavily indexing on a Hermitian then. They should mainly be used for dispatching.

I just realized that I haven't written a full method yet.

@andreasnoack
Copy link
Member Author

The printing is prettier now

julia> A=randn(5,3)
5x3 Float64 Array:
  0.928881  -0.476071   0.982468 
  0.420802   0.552705  -0.329329 
 -1.45427    1.61268   -1.82751  
 -1.28442   -1.03301   -0.816368 
 -0.435444  -0.336586   0.0642692

julia> qr(A)[:Q]
5x3 Float64 QRDenseQ:
 -0.415653   0.136119  -0.0579582
 -0.188299  -0.318782   0.750931 
  0.65075   -0.642555  -0.0810155
  0.574747   0.649197   0.454656 
  0.194851   0.213344  -0.468475 

julia> full(qr(A)[:Q])
5x3 Float64 Array:
 -0.415653   0.136119  -0.0579582
 -0.188299  -0.318782   0.750931 
  0.65075   -0.642555  -0.0810155
  0.574747   0.649197   0.454656 
  0.194851   0.213344  -0.468475 

julia> Hermitian(A'A)
3x3 Float64 Hermitian:
  4.99413  -1.08152   4.45226
 -1.08152   4.31327  -2.77524
  4.45226  -2.77524   5.08406

julia> full(Hermitian(A'A))
3x3 Float64 Array:
  4.99413  -1.08152   4.45226
 -1.08152   4.31327  -2.77524
  4.45226  -2.77524   5.08406

@lindahua
Copy link
Contributor

+1

@ViralBShah
Copy link
Member

We need diag(CholeskyDense, Int) for tests to pass.

@andreasnoack
Copy link
Member Author

I did diag(chol(Matrix)[:U]) in the logdet and that solved it.

@ViralBShah
Copy link
Member

Thanks. I was just thinking of doing the same.

@andreasnoack
Copy link
Member Author

I am soon adding methods for triangular multiplication and I think it would be useful to cover multiplication and division for the new dispatch types, but still it is very likely that someone will encounter missing methods. If there was a DenseMatrix or StridedMatrix abstract type this wouldn't really be a problem. When the faster methods existed they you be used and if not you could fall back on the general methods for matrices. Right now Triangular and Hermitian are subtypes of AbstractMatrix and the only gain seems to be a printing method.

@simonbyrne
Copy link
Contributor

Is there any chance of adding a Diagonal type as well? I realise there is diagmm, but it's much more elegant to be able to write A * D * B instead of A * diagmm(D,B) (or blatantly abuse of bsxfun as is often done in Matlab code).

@ViralBShah
Copy link
Member

I think we should also split linalg_dense.jl into a few files to make it manageable. I was thinking of just moving all the special matrix types into their own files.

In fact, it may be useful to have a linalg/ directory and move all the linear algebra stuff in there.

@ViralBShah
Copy link
Member

Also see #2345. Perhaps we can experiment with a different type hierarchy in here.

@andreasnoack
Copy link
Member Author

@ViralBShah I agree on splitting linalg_dense.jl into smaller files in a directory. It would be easier to get an overview. I also think it would be a good idea to try out a DenseArray abstract type on this branch. However, I have tried to play with the source and kick in a DenseArray between Array and AbstractArray but the bootstrap fails during io.jl. I cannot really see why it should fail there, but generally in think there is lot I don't see when looking at the c source.

@dmbates
Copy link
Member

dmbates commented Feb 27, 2013

Please keep the extras/suitesparse.jl from the db/suitesparse branch in mind when considering the new design of the linalg factorization functions. I am somewhat in stasis at present while waiting to see what the convention for factorizations will be. The sparse LU and Cholesky factorizations are available once the names are straightened out. I can also add a sparse QR at that time.

@ViralBShah
Copy link
Member

The functions that are being removed need to be included in deprecated.jl.

@StefanKarpinski
Copy link
Sponsor Member

There seems to be some confusion about what deprecated means: a function is only deprecated if it still works but shouldn't be used; if you just delete something, it isn't deprecated, it's just gone.

@dmbates
Copy link
Member

dmbates commented Mar 13, 2013

@ViralBShah Can we lift the methods for the decrement, decrement!, increment and increment! functions to a higher level than SuiteSparse. I am trying to separate UMFPACK from CHOLMOD and those functions are the only overlaps.

Things were getting too unwieldy trying to keep both together.
For the time being I have kept all the const definitions in suitesparse_h.jl
@ViralBShah
Copy link
Member

@dmbates The suitesparse tests need to be updated, as the suitesparse tests are failing on this branch. The Base.LinAlg.SuiteSparse name no longer exists. I tried to add using LinAlg.CHOLMOD and changing the names, but the tests continue to fail.

@ViralBShah
Copy link
Member

This is for @StefanKarpinski and others following this from a distance. Essentially, we are retaining the the interface from 0.1, and there is no interface change:

  1. This means that matlab compatible functions stay - lu, chol, etc.
  2. The factorization interface is accessed with lufact, lufact!, cholfact, cholfact!, etc.

New capabilities here are:

  1. Easily pull out factors using indexing - like lufact(A)[:L].
  2. New matrix types for solvers - Hermitian, etc.
  3. Better support for sparse factorizations.
  4. Better arpack support.

When we merge this, we will do a detailed writeup of all the changes.

@dmbates
Copy link
Member

dmbates commented Mar 14, 2013

@ViralBShah Sorry about the failure of the suitesparse tests. I am working on a fix now. Let me know if you want one immediately and I will comment out the tests that are failing.

@timholy
Copy link
Sponsor Member

timholy commented Mar 14, 2013

I haven't looked at the code, but your bullet list sounds good to me.

Commented out a couple of tests.  The cholmod_sdmult calls throw exceptions
but I haven't been able to determine why.
Also added a show method for the CholmodFactor type.
CholmodSparse/CholmodDense multiplication and \ now working.
Added tests but still need a lot more.
@ViralBShah
Copy link
Member

@andreasnoackjensen When do you think we can be ready to merge this into master? If the dense stuff is ready, we can comment out the failing suitesparse tests and merge. We can keep this branch around until @dmbates finishes his suitesparse work, and do another merge into master then.

@dmbates
Copy link
Member

dmbates commented Mar 15, 2013

@ViralBShah @andreasnoackjensen The current versions of umfpack.jl and cholmod.jl do pass the suitesparse tests. For my use I had the tests print some information to STDOUT. I can push a version of test/suitesparse.jl now to suppress that.

Do you plan to merge this branch (which could be very messy) or just pull in the modified files? I ask because there are many commits of the SuiteSparse interface code that are interim versions. If you are going to merge, those commits should be squashed.

@dmbates
Copy link
Member

dmbates commented Mar 15, 2013

A general design issue for the CHOLMOD module: In the past I had defined the most basic method with the c_Cholmod* arguments and then defined the methods for Cholmod* arguments by calling the basic method on foo.c field. Now I don't think that is a good idea. Most of the C functions return a Ptr{c_Cholmod_} but it is not a good idea to have either the pointer or the c_Cholmod_ object created with unsafe_ref running around loose because the ownership of the memory addressed by such objects is unclear. Constructing a Cholmod* object from a Ptr{c_Cholmod*} transfers responsibility for freeing the memory to the finalizer of the Julia object. A programmer who wants fine-grained control of the memory can call the C functions directly.

@StefanKarpinski
Copy link
Sponsor Member

My main question/concern about this change of direction is that we previously decided that it was too much to keep both the Matlab-compatible versions of these functions and the new factorization-returning versions. So what caused the change in thinking here? Why was that deemed to be no longer too much? Is this a transitional change or is this the ultimate design we're thinking to end up with?

In particular, how does all of this relate to the realization that emerged from that other thread that some "methods" of the Matlab functions compute a factorization whereas other methods compute a factorization-related artifact, which is in-and-of-itself useful, but does not behave equivalently to the original matrix as a linear operator? I'm thinking of chol here, but it's applicable to other factorization-related functions.

My final concern is that "retaining compatibility with Matlab" isn't even possible for us since we can't dispatch on the number of output arguments. Which leaves us with the problem of which method of each Matlab factorization function to retain compatibility with. This was one of the arguments for using only an indexing interface since we simply cannot express the output polymorphism that Matlab can, due to lack of overloading based on the number of return values.

and definitions of constants.

The biggest change in cholmod.jl is defining and using a macro "chm_nm"
instead of writing out all those tedious names explicitly.
@ViralBShah
Copy link
Member

I had originally proposed removing the matlab-like versions, but there was no clear consensus on that, with many wanting that interface. So, in 0.1, we retained it, and that continues to be the case here. The matlab-like interface is not fully vararg compatible, but is not too bad either.

I'd much rather focus at this point on making the factorization interface much better.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Mar 16, 2013

after an unsafe_ref copy operation, the object returned is owned by julia. the data at the original ptr is not referenced again and can be immediately freed or mutated without effect. although, any pointers inside of these objects are still up to the programmer to manage.

On Mar 15, 2013, at 10:41 AM, dmbates notifications@github.com wrote:

A general design issue for the CHOLMOD module: In the past I had defined the most basic method with the c_Cholmod* arguments and then defined the methods for Cholmod* arguments by calling the basic method on foo.c field. Now I don't think that is a good idea. Most of the C functions return a Ptr{c_Cholmod_} but it is not a good idea to have either the pointer or the c_Cholmod_ object created with unsafe_ref running around loose because the ownership of the memory addressed by such objects is unclear. Constructing a Cholmod* object from a Ptr{c_Cholmod*} transfers responsibility for freeing the memory to the finalizer of the Julia object. A programmer who wants fine-grained control of the memory can call the C functions directly.


Reply to this email directly or view it on GitHub.

@andreasnoack
Copy link
Member Author

@ViralBShah and @StefanKarpinski. People have given their opinions and there is not full agreement, so it is really up to you now to decide on the design or a design path.

@ViralBShah I only have the two issues mentioned earlier. Maybe they disappear when this branch is merged. Also, I haven't removed Dense which we might want to do before the merge. Besides that, I think it should be good to go for my part of this work.

@ViralBShah
Copy link
Member

@andreasnoackjensen Do let me know when ready to merge. The Dense name may be ok to keep for now, since we are not planning to use the constructors and instead are using the *fact versions.

As for the API, let's not have this pull request depend on it. The API can be easily changed if necessary with little effort once this is merged.

@ViralBShah
Copy link
Member

@dmbates There are no more suitesparse test failures. Is this generally ok to merge?

@dmbates
Copy link
Member

dmbates commented Mar 16, 2013

@ViralBShah Yes, okay to merge.

ViralBShah added a commit that referenced this pull request Mar 16, 2013
WIP:New design of linalg (factorizations)
@ViralBShah ViralBShah merged commit d19ad7c into master Mar 16, 2013
@dmbates
Copy link
Member

dmbates commented Mar 16, 2013

@ViralBShah We should merge my latest change on this branch into master as well.

@ViralBShah
Copy link
Member

Will do.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.