[MRG] Be compatible with newer scikit-learn #199
Conversation
wdevazelhes
changed the title
wdevazelhes
changed the title
| @@ -88,10 +87,6 @@ def check_input(input_data, y=None, preprocessor=None, | |||
| is originally 1D and ``ensure_2d`` is True. Setting to 0 disables | |||
| this check. | |||
|
|
|||
| warn_on_dtype : boolean (default=False) | |||
There was a problem hiding this comment.
scikit-learn 0.21 deprecates warn_on_dtype (see: scikit-learn/scikit-learn#13324). Since we never use it (except to test it works), I suggest we get rid of it
wdevazelhes
changed the title
wdevazelhes
requested review from
perimosocordiae,
bellet and
nvauquie
and removed request for
perimosocordiae
test/test_base_metric.py
Outdated
| @@ -5,86 +5,108 @@ | |||
| from sklearn import clone | |||
| from sklearn.utils.testing import set_random_state | |||
| from test.test_utils import ids_metric_learners, metric_learners | |||
| import string | |||
There was a problem hiding this comment.
Is this import used?
test/test_base_metric.py
Outdated
| def remove_spaces(s): | ||
| s = s.replace(' ', '') | ||
| s = s.replace('\n', '') | ||
| return s |
There was a problem hiding this comment.
I'd rather use a regex here:
return re.sub('\s+', ' ', s)
There was a problem hiding this comment.
That's right, it's better
You meant return re.sub('\s+', '', s) to remove the spaces right ?
There was a problem hiding this comment.
Yeah, should be an empty replacement string.
test/test_base_metric.py
Outdated
|
|
||
| def test_lmnn(self): | ||
| self.assertRegexpMatches( | ||
| str(metric_learn.LMNN()), | ||
| r"(python_)?LMNN\(convergence_tol=0.001, k=3, learn_rate=1e-07, " | ||
| r"max_iter=1000,\n min_iter=50, preprocessor=None, " | ||
| r"regularization=0.5, use_pca=True,\n verbose=False\)") | ||
| r"max_iter=1000,\n(\ +)min_iter=50, preprocessor=None, " |
There was a problem hiding this comment.
\s+ is a bit more readable, IMO
There was a problem hiding this comment.
I agree, it's better, I didn't know \s
test/test_base_metric.py
Outdated
| ITML_Supervised(A0=None, bounds='deprecated', convergence_threshold=0.001, | ||
| gamma=1.0, max_iter=1000, num_constraints=None, | ||
| num_labeled='deprecated', preprocessor=None, verbose=False) | ||
| """.strip('\n')) | ||
| """.strip('\n'))) |
There was a problem hiding this comment.
No need to strip newlines if you're also calling remove_spaces(), right?
|
Yes, @perimosocordiae here's the issue: #166 |
|
Thanks for the review @perimosocordiae, I just addressed your comments |
test/test_base_metric.py
Outdated
| str(metric_learn.LMNN()), | ||
| r"(python_)?LMNN\(convergence_tol=0.001, k=3, learn_rate=1e-07, " | ||
| r"max_iter=1000,(\s+)min_iter=50, preprocessor=None, " | ||
| r"regularization=0.5, use_pca=True,(\s+)verbose=False\)") |
There was a problem hiding this comment.
Don't need to wrap the whitespace matchers in parens.
|
+1 to merge when the travis results come back. |
|
The results are green, merging |
Scikit-learn just released their last version 0.21, which does not support python<3.5, so I think we can update the travis script to download the 0.20.3 for python3.4 and python2.7, but still use the newest release for python3.6 (so that we test the behaviour on the new scikit-learn), what do you think ?
Also, there were a few changes that need to be made to work with scikit-learn's new version, which this PR addresses