Skip to content

[MRG] Move transformer_from_metric to util #151

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jan 7, 2019

Conversation

bellet
Copy link
Member

@bellet bellet commented Jan 7, 2019

Fixes #146

Copy link
Member

@wdevazelhes wdevazelhes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems great !

@perimosocordiae perimosocordiae merged commit 8ffd998 into scikit-learn-contrib:master Jan 7, 2019
@perimosocordiae
Copy link
Contributor

LGTM, merged.

@perimosocordiae
Copy link
Contributor

Side note: we may want to find a more performant way to check if a matrix is diagonal, other than allclose(m, diag(diag(m))). I wonder if scikit-learn has something along those lines?

@perimosocordiae
Copy link
Contributor

Then again, the metric is a d by d matrix, so it's not a big deal in most cases.

@wdevazelhes
Copy link
Member

Yes, according to this answer and this answer, maybe a good way in numpy would be to remove the diagonal and check if the matrix is zero, like:
np.fill_diag(m, 0); not m.any() (but apparently any does not short circuit when finding a not zero element as noted in the Stackoverflow answer (but then this answer uses numba to do a short-circuit function)
Plus I don't know if there is a way to test if numbers are close to 0 in numpy
And yes maybe scikit-learn has some helper function to do that

wdevazelhes pushed a commit to wdevazelhes/metric-learn that referenced this pull request Jan 24, 2019
wdevazelhes pushed a commit to wdevazelhes/metric-learn that referenced this pull request Jan 24, 2019
@bellet bellet deleted the remove_method branch March 14, 2019 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants