search

scikit-learn-contrib / metric-learn

Metric learning algorithms in Python
http://contrib.scikit-learn.org/metric-learn/
MIT License
1.4k stars 234 forks source link

Drop support for python 2 and python 3.5 #291

Closed perimosocordiae closed 4 years ago

perimosocordiae commented 4 years ago

Fixes gh-166, and resolves some errors related to newer versions of scikit-learn.

perimosocordiae commented 4 years ago

The Travis failures are due to an inconsistency in the way scikit-learn prints the __repr__ of its transformers. In the version that supports py3.5, it uses the old behavior (print all params), but in newer versions (py3.6+) it only prints params that have non-default values.

Maybe this is a sign that we should drop py3.5 in this PR as well? @bellet any thoughts?

bellet commented 4 years ago

sklearn stopped supporting Python 3.5, and this is also the case for our optional dependency skggm, see #166 So I think I'm for dropping py35 as well.

If we go for this we should probably update the travis builds to include one without skggm, to make sure the code is tested in this setup

We should also simplify the skggm-related instructions in the builds and the README/doc as we should be able to just use the last version instead of a specific commit, am I right @wdevazelhes ?

wdevazelhes commented 4 years ago

I agree that it makes sense to drop py35

We should also simplify the skggm-related instructions in the builds and the README/doc as we should be able to just use the last version instead of a specific commit, am I right @wdevazelhes ?

I think there was some undeterministicness problems still present in the current release of skggm (see https://github.com/scikit-learn-contrib/metric-learn/issues/272), which fix (https://github.com/skggm/skggm/commit/a0ed406586c4364ea3297a658f415e13b5cbdaf8) hasn't been released yet... However skggm doesn't seem very active... Also, @bellet since you fixed scikit-learn's issues with graphical lasso (https://github.com/scikit-learn/scikit-learn/pull/13276), maybe now the cases where skggm is actually useful are very rare? (or inexistant ? I don't remember exactly but I think the cases to look at are the non SPD ones: cf. https://github.com/scikit-learn-contrib/metric-learn/blob/2d5a942c0f48a093f2e1c0dca2426e632626a9a6/metric_learn/sdml.py#L116)

bellet commented 4 years ago

I agree that it makes sense to drop py35

We should also simplify the skggm-related instructions in the builds and the README/doc as we should be able to just use the last version instead of a specific commit, am I right @wdevazelhes ?

I think there was some undeterministicness problems still present in the current release of skggm (see #272), which fix (skggm/skggm@a0ed406) hasn't been released yet... However skggm doesn't seem very active... Also, @bellet since you fixed scikit-learn's issues with graphical lasso (scikit-learn/scikit-learn#13276), maybe now the cases where skggm is actually useful are very rare? (or inexistant ? I don't remember exactly but I think the cases to look at are the non SPD ones: cf.

https://github.com/scikit-learn-contrib/metric-learn/blob/2d5a942c0f48a093f2e1c0dca2426e632626a9a6/metric_learn/sdml.py#L116 )

Nope, I don't think my fix changed much about this because the problem was only occurring in 2D. So let's keep things the same regarding skggm for now?