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

Fix failing tests in last build #270

Closed bellet closed 4 years ago

bellet commented 4 years ago

The problem is due to a deprecation warning for check_is_fitted in the new version of sklearn (see below)

This PR fixes this by not using the attributes argument when calling check_is_fitted.

bellet commented 4 years ago

The warning raised in the failing tests is from sklearn (see https://github.com/scikit-learn/scikit-learn/pull/14545): Passing attributes to check_is_fitted is deprecated and will be removed in 0.23. The attributes argument is ignored.

check_is_fitted now only checks the presence of any fitted attribute (ending with a trailing underscore)

It should be fine for us to simply remove the passed attributes. Will do

bellet commented 4 years ago

Removing the passed attributes does make the Python > 3.4 tests pass but makes the other fail, as sklearn dropped support for Python < 3.5.

One way to fix this would be to keep the attributes and relax the tests to let this particular warning go through, but this will make the code break when sklearn releases its next version.

So I would favor a solution based on checking Python version with sys.version_info and call check_is_fitted accordingly. What do you think? @perimosocordiae @terrytangyuan @wdevazelhes

BTW this is an example of the problems we are likely to see more and more as we continue to support Python < 3.5 while sklearn does not. I think we should also drop support for Python < 3.5 in the next release, see also discussion in #166

bellet commented 4 years ago

I implemented the strategy proposed above and all tests are now passing, so I think it is ready to merge.