Greg, Apologies that I've not had more time on this, I started a new job that doesn't afford it at the moment, :(.
I would say that a single implementation, with or without pivoting, makes sense to save confusion. Your clearer code would be a good choice for maintainability. One possible word of caution with replacing the current implementation with a pivoting version is that current users of the QRDecomposition may not expect a third factor, namely P, particularly in the solvers. This is not really a problem, but it should be well documented as a change to v2 to save confusion and perhaps method names, ie testAEqualQR should be changed to make this clear. Additionally, any pivoting QRDecomposition class should also have a getRank() method since it is after all 'rank-revealing' and in most (or all?) cases it would be more efficient than SingularValueDecomposition.getRank(). Chris. On 5 October 2011 05:05, Greg Sterijevski <gsterijev...@gmail.com> wrote: > My apologies! Forgot to tag the subject line. > > On Tue, Oct 4, 2011 at 8:35 PM, Greg Sterijevski <gsterijev...@gmail.com > >wrote: > > > Hello all, > > > > A while back I was interested in being able to do pivoting qr > > decomposition. I noticed that Chris Nix submitted a patch, but he > indicated > > that he had more work to do (testing and filling in functionality). In > the > > discussion around this, Luc suggested that I look at the QR decomposition > in > > Levenberg-Marquardt. I did just that a few days ago. The code was very > clear > > and nicely written (kudos to Luc). So, I copied the routine and made a > new > > PivotingQRDecomposition class. The class is intended as a "drop in > > replacement" for QRDecomposition. I also copied the QRSolverTest and > > QRDecompositionTest. With the exception of testUnderdetermined in the > solver > > test and testAEqualQR in QRDecompositionTest, the tests are unchanged > (and > > all pass!). With testUndertermined, the "zeroed" rows of the solution > matrix > > are interspersed throughout the matrix (because of pivoting). So I change > > the test to count all the rows that have zero norms, and check that it is > > the correct number. In testAEqualQR, I added a multiplication by the > > permutation matrix. > > > > What is the best way to proceed? I don't want to trounce the additions > that > > Chris made, but it looks like Chris has more sophisticated classes in > mind. > > I don't see this proposed change competing with his. Does it make sense > to > > bring back QRDecomposition interface (sorry Sebastien)? We can then keep > > both implementations until we are satisfied the pivoting one works. > > > > Thoughts? > > > > -Greg > > >
