WIP: Refactors QR and Cholesky factorization codes #4621

jiahao · 2013-10-23T23:35:51Z

The primary goal of this refactor is to subsume the pivoted QR and Cholesky factorizations into the functions that previously only computed the ordinary (non-pivoted) factorizations

Deprecates qrp, qrpfact, qrpfact, cholpfact, cholpfact!
Adds positional pivoted::Bool argument to qr, qrfact, qrpfact!, cholfact, and cholfact!
Reorganizes QR factorizations section so that methods common to QR, QRPackedQ, QRPivoted, QRPivotedQ are grouped together.

Keno · 2013-10-23T23:37:19Z

base/linalg/factorization.jl

@@ -384,6 +409,7 @@ end
 (\)(A::QRPivoted, B::StridedMatrix) = (\)(A, B, sqrt(eps(typeof(real(B[1])))))[1]
 (\)(A::QRPivoted, B::StridedVector) = (\)(A, reshape(B, length(B), 1))[:]

+<<<<<<< HEAD


This doesn't look like it should be here.

ViralBShah · 2013-10-24T02:49:10Z

You are probably planning on it anyways, but we need to update the docs too.

IainNZ · 2013-10-24T13:38:59Z

Is this going into 0.2? Seems like a big change post-rc!

ViralBShah · 2013-10-24T14:19:08Z

A bit big - but better to go in now rather than later. Also, the pivoted routines are not widely used, so hopefully the impact will be small.

StefanKarpinski · 2013-10-24T21:46:04Z

cc: @ViralBShah, @dmbates, @IainNZ – please review.

ViralBShah · 2013-10-24T22:13:30Z

It is a typo in the issue description. The pivoted versions of cholesky are deprecated.

andreasnoack · 2013-10-24T23:08:13Z

base/linalg/factorization.jl

    if nr == 0 return zeros(0, nrhs), 0 end
    ar = abs(A.hh[1])
-    if ar == 0 return zeros(nr, nrhs), 0 end
+    if ar < eps(typeof(ar)) return zeros(nr, nrhs), 0 end


I don't think you are right here. If the matrix is scaled such that all the elements are small this criterion could make it look as a rank zero matrix. The old criterion is taken from the LAPACK source.

I admit I didn't think through this one carefully. I see the issue with the new line, but returning zeros only if the householder reflector is exactly zero doesn't seem right either. I'll revert this for now, but this should be reviewed.

ViralBShah · 2013-10-25T00:32:34Z

I think it would be better if this was a named parameter so that we would have to do qr(A, pivoted=true), to make the code more readable.

stevengj · 2013-10-25T01:00:54Z

(I don't think it makes sense for this to go into 0.2 at this stage. It's not a bug fix, and probably few people are using pivoted QR so it's not an essential feature, and we are still arguing over the interface.)

IainNZ · 2013-10-25T01:49:18Z

Agreed

ViralBShah · 2013-10-25T02:15:39Z

Agree - this should be 0.3.

jiahao · 2013-10-26T00:58:49Z

@ViralBShah I agree that a named argument is more sensible. Are named arguments compatible with varargs? There are a bunch of methods like

cholfact!(A::StridedMatrix, uplo::Symbol=:U, pivoted::Bool=false, args...)

(and similarly for QR) that take a variable number of arguments but don't seem to do anything with them. Can we just get rid of these particular methods? Why were they there in the first place?

- Deprecates qrp, qrpfact and qrpfact! - Adds trailing pivoted::Bool option to qr, qrfact and qrpfact! - Reorganizes QR factorizations section so that methods common to QR, QRPackedQ, QRPivoted, QRPivotedQ are grouped together. - Remove improperly cleaned up blob from merge - Replace methods defining default parameters with methods with default parameter values - Rename QRPivoted.jpvt to QRPivoted.pivots

Revert A.hh[1] < eps to A.hh[1] == 0 testing for (\)(QRPivoted)

ViralBShah · 2013-11-20T08:23:24Z

bump. We should merge this once the linalg refactor is completed.

ViralBShah · 2013-11-25T04:46:15Z

@andreasnoackjensen Could you comment on @jiahao 's question above? It seems like we could get rid of the variable number of arguments.

andreasnoack · 2013-11-25T06:18:02Z

The varargs serve a purpose for most of the factorizations, e.g.

cholfact!(A::StridedMatrix, args...) = cholfact!(float(A), args...)

converts non-float matrices to float matrices wether or not uplo has been supplied. The presence of varargs in the QR might seem a bid weird, but the fancy algorithm we are using takes some tuning parameters.

ViralBShah · 2013-12-04T03:06:54Z

@jiahao Do you think we can merge this now? I think this requires updates to the docs too, but otherwise seems largely ready.

jiahao · 2013-12-04T03:43:02Z

Let me review it given the recent linalg cleanup.

ViralBShah · 2013-12-25T18:09:21Z

I think we should get this merged before it becomes too stale!

ViralBShah · 2013-12-28T06:32:50Z

@jiahao I have some time today, and can take this up. Do you have any commits you haven't pushed?

- Deprecates qrp, qrpfact and qrpfact! - Adds trailing pivoted::Bool option to qr, qrfact and qrpfact! - Reorganizes QR factorizations section so that methods common to QR, QRPackedQ, QRPivoted, QRPivotedQ are grouped together. - Remove improperly cleaned up blob from merge - Replace methods defining default parameters with methods with default parameter values - Rename QRPivoted.jpvt to QRPivoted.pivots

Revert A.hh[1] < eps to A.hh[1] == 0 testing for (\)(QRPivoted)

ViralBShah · 2013-12-29T06:06:50Z

I took at a deep look at this today. I rebased this branch to master which took quite some effort, and the tests are not passing still. Considering the effort to make pivoted as an argument to qr and chol, and the resulting complexity in the codebase, I wonder if we should just leave things as they are. We just end up with two extra functions, and that may be ok.

@jiahao @andreasnoackjensen What do you guys think?

jiahao · 2013-12-30T02:09:10Z

@ViralBShah can you push the rebased branch?

Part of the overall issue, which I haven't been too clear on, is whether it is desirable for methods that return Factorizations that are type stable with respect to the output. Does it make sense for qrfact to sometimes return QR and other times QRPivoted?

When working on this branch I also found it difficult to make changes to the factorization code, since it relies quite heavily on methods with varargs that break whenever I try to change them. I know for sure that I wasn't willing to merge this branch because of this fragility.

Perhaps the entire factorization codebase could use cleaning up. However, I don't think I can do that alone.

ViralBShah · 2013-12-30T02:59:52Z

That was what I took away as well, that we need a general codebase cleanup here. I am willing to join that effort.

… cjh/nopivot-default Conflicts: base/linalg/factorization.jl

jiahao · 2013-12-30T03:13:46Z

For example, I added a wrapper to LAPACK.gesvx!, the general linear solver that calculates an error estimate, and I have absolutely no idea where to put it.

ViralBShah · 2013-12-30T03:14:52Z

This merge is getting too screwy, and the tests are failing too. I am not sure if I should push what I have. I say, we brainstorm what we want this to look like, answering various API questions, and then just start from the latest master.

ViralBShah · 2013-12-30T03:19:22Z

Ok, I have pushed what I had, in any case, since this branch would otherwise be unusable anyways. Some stuff is failiing, which I have to track down.

…iplication. Update tests.

andreasnoack · 2013-12-30T10:49:00Z

I have fixed the last issues that made this pr fail the tests. There were only few problems. Some of them were related to promotion which is generally quite annoying and make you wish that conversions should be explicit.

What is your end 2013 opinion on pivoted as a named argument? I can do the rewrite if we want it to be named.

Regarding cleanup, I think that we should consider to split lapack.jl into more files. It is huge and the splitting would force us decide on a structure for the wrappers. Should it be by matrix or problem type?

ViralBShah · 2013-12-31T04:39:08Z

I feel that pivoted is an uncommon option, and makes sense to have as a named argument. That would also allow for other things as named arguments, like which part of a matrix to use for the cholesky factorization and such, making the code generally more readable.

I feel that lapack.jl in general contains only wrappers, and having a large monolithic file is ok, but the rest of the stuff could use a better structure. I don't have well-formed opinion on how to refactor though.

ViralBShah · 2013-12-31T04:41:04Z

@andreasnoackjensen Thanks for fixing this PR. Nice to see it all green.

ViralBShah · 2014-01-04T20:59:55Z

@andreasnoackjensen Should we merge this for now, and then do a separate PR for the cleanup and making pivoted a named argument?

What is your view on making it a named argument?

andreasnoack · 2014-01-05T15:38:07Z

I think named arguments are better and will try to update this pull request soon.

jiahao · 2014-01-11T07:38:32Z

Subsumed by #5330.

jiahao mentioned this pull request Oct 23, 2013

Make pivoting an option for QR #4616

Closed

Keno reviewed Oct 23, 2013
View reviewed changes

andreasnoack reviewed Oct 24, 2013
View reviewed changes

jiahao added 3 commits October 27, 2013 22:16

Deprecates cholpfact, cholpfact!

6348a3a

Fix dimension finding to work for StridedMatOrVec

18d0ec9

Revert A.hh[1] < eps to A.hh[1] == 0 testing for (\)(QRPivoted)

jiahao and others added 4 commits December 29, 2013 10:40

Deprecates cholpfact, cholpfact!

f30f4a7

Fix dimension finding to work for StridedMatOrVec

34d54f5

Revert A.hh[1] < eps to A.hh[1] == 0 testing for (\)(QRPivoted)

WIP

2a9e42d

Merge branch 'cjh/nopivot-default' of github.com:JuliaLang/julia into…

f321c7d

… cjh/nopivot-default Conflicts: base/linalg/factorization.jl

Merge branch 'master' into cjh/nopivot-default

ce4ab4d

Add chkfullrank. Fix inv(Cholesky). Define promotion rules for Q mult…

5fed276

…iplication. Update tests.

andreasnoack mentioned this pull request Jan 8, 2014

Deprecate cholpfact and qrpfact and introduce keyword arguments for pivoting. Make thin a keyword argument. #5330

Merged

jiahao closed this Jan 11, 2014

jiahao deleted the cjh/nopivot-default branch January 11, 2014 07:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Refactors QR and Cholesky factorization codes #4621

WIP: Refactors QR and Cholesky factorization codes #4621

jiahao commented Oct 23, 2013

Keno Oct 23, 2013

ViralBShah commented Oct 24, 2013

IainNZ commented Oct 24, 2013

ViralBShah commented Oct 24, 2013

StefanKarpinski commented Oct 24, 2013

ViralBShah commented Oct 24, 2013

andreasnoack Oct 24, 2013

jiahao Oct 26, 2013

ViralBShah commented Oct 25, 2013

stevengj commented Oct 25, 2013

IainNZ commented Oct 25, 2013

ViralBShah commented Oct 25, 2013

jiahao commented Oct 26, 2013

ViralBShah commented Nov 20, 2013

ViralBShah commented Nov 25, 2013

andreasnoack commented Nov 25, 2013

ViralBShah commented Dec 4, 2013

jiahao commented Dec 4, 2013

ViralBShah commented Dec 25, 2013

ViralBShah commented Dec 28, 2013

ViralBShah commented Dec 29, 2013

jiahao commented Dec 30, 2013

ViralBShah commented Dec 30, 2013

jiahao commented Dec 30, 2013

ViralBShah commented Dec 30, 2013

ViralBShah commented Dec 30, 2013

andreasnoack commented Dec 30, 2013

ViralBShah commented Dec 31, 2013

ViralBShah commented Dec 31, 2013

ViralBShah commented Jan 4, 2014

andreasnoack commented Jan 5, 2014

jiahao commented Jan 11, 2014

WIP: Refactors QR and Cholesky factorization codes #4621

WIP: Refactors QR and Cholesky factorization codes #4621

Conversation

jiahao commented Oct 23, 2013

Keno Oct 23, 2013

Choose a reason for hiding this comment

ViralBShah commented Oct 24, 2013

IainNZ commented Oct 24, 2013

ViralBShah commented Oct 24, 2013

StefanKarpinski commented Oct 24, 2013

ViralBShah commented Oct 24, 2013

andreasnoack Oct 24, 2013

Choose a reason for hiding this comment

jiahao Oct 26, 2013

Choose a reason for hiding this comment

ViralBShah commented Oct 25, 2013

stevengj commented Oct 25, 2013

IainNZ commented Oct 25, 2013

ViralBShah commented Oct 25, 2013

jiahao commented Oct 26, 2013

ViralBShah commented Nov 20, 2013

ViralBShah commented Nov 25, 2013

andreasnoack commented Nov 25, 2013

ViralBShah commented Dec 4, 2013

jiahao commented Dec 4, 2013

ViralBShah commented Dec 25, 2013

ViralBShah commented Dec 28, 2013

ViralBShah commented Dec 29, 2013

jiahao commented Dec 30, 2013

ViralBShah commented Dec 30, 2013

jiahao commented Dec 30, 2013

ViralBShah commented Dec 30, 2013

ViralBShah commented Dec 30, 2013

andreasnoack commented Dec 30, 2013

ViralBShah commented Dec 31, 2013

ViralBShah commented Dec 31, 2013

ViralBShah commented Jan 4, 2014

andreasnoack commented Jan 5, 2014

jiahao commented Jan 11, 2014