Hi Gilles, There are currently [at least] two qr decompositions in existence in cm. There is QRDecomposition in package linear and there is embedded in LevenbergMarquardtOptimizer the guts of a pivoting QR decomposition. So adding a PivotingQRDecomposition class in linear would initially push that to 3 implementations. Here are a couple of scenarios:
1. Eliminate QRDecomposition from linear. Eliminate LevenbergMarquardt's embed qr decompositon and have the PivotingQRDecomposition become the de jure standard. 2. Eliminate just QRDecomposition, not touching Marquardt-Levenberg. (Don't play with something that works) 3. Keep all three until the next major release, marking QRDecomposition (the class) as deprecated. -Greg On Wed, Oct 5, 2011 at 9:39 AM, Gilles Sadowski < gil...@harfang.homelinux.org> wrote: > On Wed, Oct 05, 2011 at 08:46:55AM -0500, Greg Sterijevski wrote: > > Hi Gilles, > > > > The class passes all current tests for QRDecomposition. Are you > suggesting I > > rip out the QRDecomposition from OLSMultipleRegression...etc and then > make > > sure there are no test failures? Or are you suggestion a new set of > tests? > > None of the above, I guess (IIUC). > I'm just saying that if "QR decomposition" is a well defined concept, it > should be implemented in the class "QRDecomposition" in package "linear", > and classes that need the functionality should "import" it from there. > I.e. > there should not be duplicate code (which from what I read in this thread, > seems to exist in "OLSMultipleRegression" and > "LevenbergMarquardtOptimizer"). > > > Gilles > > > > > -Greg > > > > On Wed, Oct 5, 2011 at 5:16 AM, Gilles Sadowski < > > gil...@harfang.homelinux.org> wrote: > > > > > Hi. > > > > > > > > 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. > > > > > > -1 > > > Please do the required testing. Only the "current best" implementation > > > should remain. > > > > > > > > > Regards, > > > Gilles > > > > > > --------------------------------------------------------------------- > > > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > > > For additional commands, e-mail: dev-h...@commons.apache.org > > > > > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org > For additional commands, e-mail: dev-h...@commons.apache.org > >
