[MRG] Use pseudo-inverse in Covariance #206
Conversation
metric_learn/covariance.py
Outdated
| @@ -35,11 +35,11 @@ def fit(self, X, y=None): | |||
| y : unused | |||
| """ | |||
| X = self._prepare_inputs(X, ensure_min_samples=2) | |||
| M = np.cov(X, rowvar = False) | |||
| if M.ndim == 0: | |||
| M = np.atleast2d(np.cov(X, rowvar = False)) | |||
There was a problem hiding this comment.
I added an atleast2d since we don't want the metric to be scalar
There was a problem hiding this comment.
While we're here, remove the spaces in the rowvar keyword argument.
metric_learn/covariance.py
Outdated
| M = np.cov(X, rowvar = False) | ||
| if M.ndim == 0: | ||
| M = np.atleast2d(np.cov(X, rowvar = False)) | ||
| if len(M) == 1: |
There was a problem hiding this comment.
Because of the atleast2d, this needs to change
There was a problem hiding this comment.
Can we be more explicit and check M.size == 1? Would it be possible for M to be a column vector here?
There was a problem hiding this comment.
Ah yes you're right thanks
I thought that len counted the number of elements but indeed len(np.array([[1, 2, 3]])) == 1
|
I think it should be good to merge now |
metric_learn/covariance.py
Outdated
| @@ -14,6 +14,7 @@ | |||
|
|
|||
| from .base_metric import MahalanobisMixin | |||
| from ._util import transformer_from_metric | |||
| import scipy | |||
There was a problem hiding this comment.
Style nitpick: move this import up to line 13 (after the numpy import but before the sklearn)
|
Maybe fix the small style issue so we can merge? |
Yep, sorry, it's done now I think we can merge |
Fixes #204
This PR uses the pseudo inverse in Covariance, rather than the inverse. See #204 (comment)