commons-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Sujit Pal <sujit....@comcast.net>
Subject Re: [math] Matrix decomposition API
Date Fri, 05 Dec 2008 18:00:10 GMT
FWIW, I like the calling pattern with the static DecompositionSolver
too...
   RealVector solution =
     DecompositionSolver.solve(constant, new QRDecompositionImpl(matrix));

-sujit

On Fri, 2008-12-05 at 10:37 +0100, luc.maisonobe@free.fr wrote:
> ----- "Phil Steitz" <phil.steitz@gmail.com> a écrit :
> 
> 
> > Looks good.  Just one last question / suggestion.   In the 
> > DecompositionSolver, why to we keep the argumentless decompose()
> > methods 
> > and the matrix as an instance variable?  The argumentless methods just
> > 
> > delegate to the ones that take a matrix as an argument and the
> > instance 
> > variable is otherwise unused. 
> > 
> > Phil
> 
> This also bothers me. I tried different things and ended up with that but didn't feel
comfortable with it. Since it was already very late, I commited it for review.
> 
> In addition to the fact the instance field is almost not used, the calling code is awkward:
> 
>   DecompositionSolver solver = new DecompositionSolver(matrix);
>   QRDecomposition decomposition = solver.qrDecompose();
>   RealVector solution = solver.solve(constant, decomposition);
> 
> I would prefer a one-liner. After one night sleeping, I see two better approaches. The
first one would be to remove completely the field instance, the xyzDecompose methods and change
all other methods (solve, isNonSingular ...) to static, thus making DecompositionSolver a
utility class never instantiated. The second one would be to have a DecompositionSolver that
can be instantiated but instead of storing the matrix, it would store a configuration flag
specifying which decomposition algorithm to use, in this case, the undecomposed matrix would
be passed in the solve method and decomposed on the fly.
> 
> The first approach leads to:
>    RealVector solution =
>      DecompositionSolver.solve(constant, new QRDecompositionImpl(matrix));
> 
> The second approach leads to:
>   RealVector solution =
>     new DecompositionSolver(DecompositionSolver.USE_QR).solve(constant, matrix);
> 
> I prefer the first one. The decomposition algorithm is more explicit to users and they
can reuse it. The second approach only advantage is if someone wants to configure a solver
with some algorithm type once and for all and to reuse it several time in his code. However
I think this use case is not a common one and it could be done by a suitable wrapper.
> 
> There is also one other thing I don't really like in my current implementation: the overloaded
methods with the four types of decomposition (EigenDecomposition, SingularValuesDecomposition,
QRDecomposition, LUDecomposition). However, simplifying this would require reintroducing some
common base interface and wrappers like this:
> 
>    public class Solver {
>      public RealVector solve(RealVector constant) {
>        ...
>      }
>    }
>    public class QRSolver implements Solver {
>     ...
>    }
> 
>    public class DecompositionSolver {
>      public RealVector solve(RealVector constant, Solver solver) {
>        return solver.solve(constant);
>      }
>    }
> 
> and we would end up with a DecompositionSolver that has no purpose at all! So I think
it is better to stick with the four signatures types.
> 
> I will therefore implement and commit today the first approach without the four signatures
types, just to see how it looks once implemented. It is straightforward and could be changed
afterwards if something better is found.
> 
> 
> Luc
> 
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
> For additional commands, e-mail: dev-help@commons.apache.org
> 


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@commons.apache.org
For additional commands, e-mail: dev-help@commons.apache.org


Mime
View raw message