[MRG] make transformer_from_metric more robust #191
Conversation
wdevazelhes
changed the title
|
Now that I think about it, maybe the warning is not a good thing to throw ? Maybe it would be better a verbose flag to decide whether to print the warning (which would not be anymore a warning but a regular print) ? What do you think ? (And the verbose attribute of the estimator that uses this would be given as an argument to this function) |
There was a problem hiding this comment.
As discussed I think it would be better to only allow symmetric PSD matrices as input and throw an error if it is not (but allowing for some tolerance for numerical errors where some eigenvalues are very close to zero but negative). Also, for the diagonal case, we can go for the safe solution of simply checking if the off diagonal terms are exactly zero (which will cover the most concrete case where the matrix is learned to be diagonal in the first place as in MMC with diagonal=True) and treat the matrix as non-diagonal otherwise.
|
I agree, I just pushed a more recent version that takes into account these remarks |
There was a problem hiding this comment.
Some nitpicks.
Also the docstring seems overly detailed. I think "Computes the transformation matrix from the Mahalanobis matrix, i.e. the matrix L such that metric=L.T.dot(L)." is enough, the rest could be put as comments inside the function to explain what is being done?
metric_learn/_util.py
Outdated
|
|
||
| tol : float, optional | ||
| Negative eigenvalues above - tol are considered zero. If | ||
| tol is None, and w are `metric`'s eigenvalues, and eps is the |
There was a problem hiding this comment.
"and w are metric's eigenvalues" is not needed
metric_learn/_util.py
Outdated
| """ | ||
| if tol is None: | ||
| tol = w.max() * len(w) * np.finfo(w.dtype).eps | ||
| if any(w[w < 0] < - tol): |
There was a problem hiding this comment.
although it actually prevents that anything is done in case the provided tol is negative (which is not supposed to be the case according to the docstring but is not checked)
There was a problem hiding this comment.
That's right, I'll let it be w < - tol and add a quick test to check that tol is positive
metric_learn/_util.py
Outdated
|
|
||
| tol : positive float, optional | ||
| Eigenvalues of `metric` between 0 and - tol are considered zero. If tol is | ||
| None, and w are `metric`'s eigenvalues, and eps is the epsilon value for |
There was a problem hiding this comment.
How to explain how the tolerance is set here then without talking about the eigenvalues ? Though maybe an explanation talking about the highest eigenvalue could be better ? Like:
Eigenvalues of `metric` between 0 and - tol are considered zero. If tol is
None, and w_max is `metric`'s largest eigenvalue, and eps is the epsilon
value for datatype of w, then tol is set to w_max * metric.shape[0] * eps.
There was a problem hiding this comment.
nevermind, indeed here we need to mention eigenvalues
the suggested reformulation looks good
| P = ortho_group.rvs(7, random_state=rng) | ||
| M = P.dot(D).dot(P.T) | ||
| with pytest.raises(ValueError) as raised_error: | ||
| transformer_from_metric(M) |
There was a problem hiding this comment.
here you could also easily test that the error is raised in the diagonal case, for instance when calling transformer_from_metric(D)
|
Thanks for the review @bellet, I addressed all the comments, there's just |
|
LGTM! |
Fixes #175
This PR fixes the problem we had with
transformer_from_metric, by checking if the matrix is diagonal depending on the relative gap between the maximum absolute value of the outer diagonal coeffs, and the minimum absolute value of the diagonal coefficients, rather than what was done before.It is also more robust for detecting whether to use Cholesky or the eigendecomposition: instead of computing the determinant to check if the matrix is not definite, it tries to do Cholesky and if an error is returned it does the eigendecomposition. Indeed, maybe the determinant can be not null if there is a very small eigenvalue (
v=1e-5) (that should be considered as null), but a lot of big eigenvalues (see one example in the test)It also raises an error message when the input is not symmetric and a warning when switching from the Cholesky decomposition to the eigendecompostion if the Cholesky decomposition is not doable.
TODO:
vfor which cholesky will fail, and from it build the example where the determinant is not "np.close" to zero