{"action":"created","issue":{"url":"https://api.github.com/repos/metric-learn/metric-learn/issues/168","repository_url":"https://api.github.com/repos/metric-learn/metric-learn","labels_url":"https://api.github.com/repos/metric-learn/metric-learn/issues/168/labels{/name}","comments_url":"https://api.github.com/repos/metric-learn/metric-learn/issues/168/comments","events_url":"https://api.github.com/repos/metric-learn/metric-learn/issues/168/events","html_url":"https://github.com/metric-learn/metric-learn/pull/168","id":406348549,"node_id":"MDExOlB1bGxSZXF1ZXN0MjUwMDY2MDYw","number":168,"title":"[WIP] This PR fits a threshold for tuples learners to allow a predict (and scoring) on pairs","user":{"login":"wdevazelhes","id":31916524,"node_id":"MDQ6VXNlcjMxOTE2NTI0","avatar_url":"https://avatars2.githubusercontent.com/u/31916524?v=4","gravatar_id":"","url":"https://api.github.com/users/wdevazelhes","html_url":"https://github.com/wdevazelhes","followers_url":"https://api.github.com/users/wdevazelhes/followers","following_url":"https://api.github.com/users/wdevazelhes/following{/other_user}","gists_url":"https://api.github.com/users/wdevazelhes/gists{/gist_id}","starred_url":"https://api.github.com/users/wdevazelhes/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/wdevazelhes/subscriptions","organizations_url":"https://api.github.com/users/wdevazelhes/orgs","repos_url":"https://api.github.com/users/wdevazelhes/repos","events_url":"https://api.github.com/users/wdevazelhes/events{/privacy}","received_events_url":"https://api.github.com/users/wdevazelhes/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"assignees":[],"milestone":null,"comments":6,"created_at":"2019-02-04T14:15:29Z","updated_at":"2019-02-21T09:27:35Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/metric-learn/metric-learn/pulls/168","html_url":"https://github.com/metric-learn/metric-learn/pull/168","diff_url":"https://github.com/metric-learn/metric-learn/pull/168.diff","patch_url":"https://github.com/metric-learn/metric-learn/pull/168.patch"},"body":"Fixes #131 \r\nFixes #165  \r\n\r\nFinally, we decided that it would be good to have at least  a  minimal implementation of threshold calibration inside metric-learn, so that `_PairsClassifiers` can have a threshold hence a predict directly out of the box, without the need for a MetaEstimator. A MetaEstimator could however be used outside the algorithm for more precise threshold calibration (with cross-validation). \r\n\r\nThe following features should be implemented: \r\n- [ ] We should have two methods for `_PairsClassifiers`: `set_threshold()` and `calibrate_threshold(validation_set, method='max_tpr', args={'min_tnr': 0.1})`. `set_threshold` will set the threshold to a hard value, and `calibrate_threshold` will take a validation set and a method ('accuracy', 'f1'...) and will find the threshold which optimizes the metric in `method` on the validation set.\r\n- [ ] At fit time, we should either have a simple rule to set a threhold (for instance median of distances, or mean between positive pairs distances mean and negative pairs distances mean), or we should return `calibrate_threshold(trainset)`, and in this case also raise a warning at the end that says that the threshold has been fitted on the trainset, so we should check scikit-learn's utilities for calibration to have a calibration less prone to overfitting. Also in this case we could allow to put arguments in `fit` like `fit(pairs, y, threshold_method='max_tpr', threshold_args={'min_tnr': 0.1})\r\n- [ ] The following scores should be implemented: \r\n  - 'accuracy'\r\n  - ‘f_beta’\r\n  - ‘max_tpr’\r\n  - 'max_tnr'\r\nSee scikit-learn's calibration PR (https://github.com/scikit-learn/scikit-learn/pull/10117) for more details, and the documentation of it here https://35753-843222-gh.circle-artifacts.com/0/doc/modules/calibration.html\r\n- [ ] For some estimators for which a natural threshold exist (like `ITML`: the mean between the lower threshold and the higher threshold), we should put this threshold\r\n\r\n## Questions: \r\n\r\n- Should we do the same thing for QuadrupletsLearners ? (the natural threshold is 0, so I don't think it would make as much sense here, and users would maybe rather use the future meta estimator from scikit-learn's PR https://github.com/scikit-learn/scikit-learn/pull/10117), but since we will already have it implemented for pairs, and maybe for coherence, we could have it also for Quadruplets Learners\r\n\r\n## TODO: \r\n- [x] Implement tests that check that we can use custom scores in cross val (cf #165). This needs to have a predict hence is related to this PR. \r\n- [ ] Implement API tests (that the behaviour is as expected)\r\n- [ ] Implement numerical tests (on examples where we know the f_1 score etc\r\n- [ ] Implement the actual method (cf. features above)\r\n- [ ] Add this in the doc: also talk in the doc about the `CalibratedClassifierCV`\r\n- [ ] Think about which scoring make sense with quadruplets and what impact is has on the code\r\n- [ ] Use/Adapt `CalibratedClassifierCV` or an equivalent for quadruplets\r\n- [ ] Maybe test CalibratedClassifierCV that it returns a coherent value (like all that have predict_proba = 0.8 have indeed 80% success) "},"comment":{"url":"https://api.github.com/repos/metric-learn/metric-learn/issues/comments/465926094","html_url":"https://github.com/metric-learn/metric-learn/pull/168#issuecomment-465926094","issue_url":"https://api.github.com/repos/metric-learn/metric-learn/issues/168","id":465926094,"node_id":"MDEyOklzc3VlQ29tbWVudDQ2NTkyNjA5NA==","user":{"login":"wdevazelhes","id":31916524,"node_id":"MDQ6VXNlcjMxOTE2NTI0","avatar_url":"https://avatars2.githubusercontent.com/u/31916524?v=4","gravatar_id":"","url":"https://api.github.com/users/wdevazelhes","html_url":"https://github.com/wdevazelhes","followers_url":"https://api.github.com/users/wdevazelhes/followers","following_url":"https://api.github.com/users/wdevazelhes/following{/other_user}","gists_url":"https://api.github.com/users/wdevazelhes/gists{/gist_id}","starred_url":"https://api.github.com/users/wdevazelhes/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/wdevazelhes/subscriptions","organizations_url":"https://api.github.com/users/wdevazelhes/orgs","repos_url":"https://api.github.com/users/wdevazelhes/repos","events_url":"https://api.github.com/users/wdevazelhes/events{/privacy}","received_events_url":"https://api.github.com/users/wdevazelhes/received_events","type":"User","site_admin":false},"created_at":"2019-02-21T09:27:35Z","updated_at":"2019-02-21T09:27:35Z","author_association":"CONTRIBUTOR","body":"> can't we rather keep the threshold positive (i.e., which works with actual distances and not negative distances), and change the predict accordingly? It feels weird to have a negative threshold\r\n\r\nYes I agree, I was hesitating between both, but I guess a negative threshold is weird indeed"}}
{"actor":{"display_login":"wdevazelhes"}}