Skip to content

Fix failing tests in last build #270

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 7 commits into from
Dec 13, 2019

Conversation

bellet
Copy link
Member

@bellet bellet commented Dec 12, 2019

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
Copy link
Member Author

bellet commented Dec 12, 2019

The warning raised in the failing tests is from sklearn (see scikit-learn/scikit-learn#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 bellet changed the title [Debug] Try to print which warnings are raised Fix failing test in last build Dec 12, 2019
@bellet bellet changed the title Fix failing test in last build Fix failing tests in last build Dec 12, 2019
@bellet
Copy link
Member Author

bellet commented Dec 13, 2019

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
Copy link
Member Author

bellet commented Dec 13, 2019

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

Copy link
Member

@terrytangyuan terrytangyuan left a comment

Choose a reason for hiding this comment

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

Great!

@terrytangyuan terrytangyuan merged commit 7819e7c into scikit-learn-contrib:master Dec 13, 2019
@bellet bellet deleted the change_test branch December 13, 2019 12:34
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.

2 participants