Re: [MATH] Repurposing a deprecated constructor in EigenDecomposition

2013-10-27 Thread Phil Steitz
On 10/23/13 8:03 PM, Ted Dunning wrote:
 On Wed, Oct 23, 2013 at 8:33 PM, Sean Owen sro...@gmail.com wrote:

 it feels a little funny just
 because then we should have similar logic for other decompositions. I
 think I remember the LU one stops early, always.

 The stopping early is definitely an option with QR.  With LU, it isn't so
 clear.


Sorry to be a little late to the party on this.  We should keep in
mind that singularity threshold means different things in the
different contexts discussed above (as Ted and others have
mentioned).  It should be provided to constructors that need it and
clearly documented (cf LU).  I like adding a boolean to Eigen so the
user can decide whether or not to stop.  I agree with Thomas that
adding a parameter to getSolver makes sense in cases where there is
no instance field (or even possible where there is, so the solver
can override).   What really needs to be improved is the
documentation on what exactly threshold means in each case where
it is used.

So basically, I am saying

0) get rid of constructor argument (i.e. depecate) where it is only
used by getSolver (QR), adding it there.  Look at all of them to see
if it makes sense to provide this parameter.
1) work around the overloading issue in EigenDecomposiiton by adding
the boolean flag (which is useful anyway)
2) crisp up doco throughout to make it clear exactly what thresholds
mean

Phil

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



[MATH] Repurposing a deprecated constructor in EigenDecomposition

2013-10-23 Thread Sean Owen
In MATH-1045 (https://issues.apache.org/jira/browse/MATH-1045) we have
discussed adding a zero threshold tolerance to EigenDecomposition just
like QRDecomposition has. This involves adding a new constructor with
a new double parameter.

Just one problem: there's already such a constructor:

/**
 * Calculates the eigen decomposition of the given real matrix.
 *
 * @param matrix Matrix to decompose.
 * @param splitTolerance Dummy parameter (present for backward
 * compatibility only).
 * @throws MathArithmeticException  if the decomposition of a general matrix
 * results in a matrix with zero norm
 * @throws MaxCountExceededException if the algorithm fails to converge.
 * @deprecated in 3.1 (to be removed in 4.0) due to unused parameter
 */
@Deprecated
public EigenDecomposition(final RealMatrix matrix,
  final double splitTolerance)
throws MathArithmeticException {
this(matrix);
}

Reusing this constructor would introduce a functionally incompatible
change, as the previous meaning of this value was different.

What are people's thoughts on proceeding anyway for a minor point
release like 3.3?
Or, best saved for 4.x?

Sean

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



Re: [MATH] Repurposing a deprecated constructor in EigenDecomposition

2013-10-23 Thread Sean Owen
Yeah it's a tricky one. I agree with the functionality, because the QR
decomposition can happily operate on a singular matrix. R will be
singular if A is, AFAIK. So the singularity threshold business belongs
more in the bit that uses R, yes.

The same could be said of the SVD. But it provides no singularity
threshold parameter. Not suggesting changing that here, let's put it
aside.

Cholesky doesn't need a singularity threshold, but takes other
thresholds in its constructor. That's OK since they are relevant to
the decomposition.

Same for LU -- it needs a singularity threshold because the LU
decomposition can proceed but only to a point if the matrix is
singular. (This is at the limit of my understanding.)

EigenDecomposition resembles QR in this respect, as far as they are
implemented here. This argues for them to treat arguments similarly.

I suppose the best thing would be to move the threshold to getSolver()
in both cases. This seems a better design, and no more trouble than,
dealing with the problem of the old deprecated constructor and putting
the arguments in the decomposition constructor in both places.

I suppose it's also reasonable to punt and not add this threshold just now.

Any more thoughts?


On Wed, Oct 23, 2013 at 1:14 PM, Thomas Neidhart
thomas.neidh...@gmail.com wrote:
 Hi Sean,

 while looking at the QRDecomposition, it is interesting that the threshold
 value provided to the constructor of QRDecomposition is only used to pass
 it on the the Solver created by getSolver().

 Imho, it would be more logical to chose the singularity threshold when
 calling getSolver() for the respective decomposition, that way we could
 also add it to the EigenDecomposition class.

 Thomas


 On Wed, Oct 23, 2013 at 1:23 PM, Sean Owen sro...@gmail.com wrote:

 In MATH-1045 (https://issues.apache.org/jira/browse/MATH-1045) we have
 discussed adding a zero threshold tolerance to EigenDecomposition just
 like QRDecomposition has. This involves adding a new constructor with
 a new double parameter.

 Just one problem: there's already such a constructor:

 /**
  * Calculates the eigen decomposition of the given real matrix.
  *
  * @param matrix Matrix to decompose.
  * @param splitTolerance Dummy parameter (present for backward
  * compatibility only).
  * @throws MathArithmeticException  if the decomposition of a general
 matrix
  * results in a matrix with zero norm
  * @throws MaxCountExceededException if the algorithm fails to
 converge.
  * @deprecated in 3.1 (to be removed in 4.0) due to unused parameter
  */
 @Deprecated
 public EigenDecomposition(final RealMatrix matrix,
   final double splitTolerance)
 throws MathArithmeticException {
 this(matrix);
 }

 Reusing this constructor would introduce a functionally incompatible
 change, as the previous meaning of this value was different.

 What are people's thoughts on proceeding anyway for a minor point
 release like 3.3?
 Or, best saved for 4.x?

 Sean

 -
 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



Re: [MATH] Repurposing a deprecated constructor in EigenDecomposition

2013-10-23 Thread Ted Dunning
On Wed, Oct 23, 2013 at 3:14 PM, Sean Owen sro...@gmail.com wrote:

 EigenDecomposition resembles QR in this respect, as far as they are
 implemented here. This argues for them to treat arguments similarly.


Actually not.  It is quite reasonable for the EigenDecomposition to stop
when singularity is reached.  This affects the shape of the eigenvector
matrix.

Perhaps add a new constructor with a double tolerance and a boolean that
says to stop early.  QR is subject to the same logic since partial QR is
often more useful than full QR with singular R.  This is the same logic as
with Cholesky since QR and Cholesky are two sides of the same coin in many
respects.


Re: [MATH] Repurposing a deprecated constructor in EigenDecomposition

2013-10-23 Thread Sean Owen
Ah I see so you can just stop computing the eigenvectors when the
eigenvalues are too small. That does seem more efficient and then the
zero values are left as literally 0. I think I should back up and
implement the patch that way.

The original question stands then -- what to do about the singularity
threshold parameter given the existence of a constructor with the
signature already? because that is where it belongs. Adding an extra
boolean flag dodges that question; it feels a little funny just
because then we should have similar logic for other decompositions. I
think I remember the LU one stops early, always.

On Wed, Oct 23, 2013 at 4:50 PM, Ted Dunning ted.dunn...@gmail.com wrote:
 On Wed, Oct 23, 2013 at 3:14 PM, Sean Owen sro...@gmail.com wrote:

 EigenDecomposition resembles QR in this respect, as far as they are
 implemented here. This argues for them to treat arguments similarly.


 Actually not.  It is quite reasonable for the EigenDecomposition to stop
 when singularity is reached.  This affects the shape of the eigenvector
 matrix.

 Perhaps add a new constructor with a double tolerance and a boolean that
 says to stop early.  QR is subject to the same logic since partial QR is
 often more useful than full QR with singular R.  This is the same logic as
 with Cholesky since QR and Cholesky are two sides of the same coin in many
 respects.

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



Re: [MATH] Repurposing a deprecated constructor in EigenDecomposition

2013-10-23 Thread Ted Dunning
On Wed, Oct 23, 2013 at 8:33 PM, Sean Owen sro...@gmail.com wrote:

 it feels a little funny just
 because then we should have similar logic for other decompositions. I
 think I remember the LU one stops early, always.


The stopping early is definitely an option with QR.  With LU, it isn't so
clear.