Chris, I hope that this isn't stepping on your toes. I was a bit concerned about that.
-Greg On Wed, Oct 5, 2011 at 2:34 AM, Chris Nix <chris....@gmail.com> wrote: > 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 > > > > > >
