The Wayback Machine - https://web.archive.org/web/20220518082419/https://github.com/scikit-learn/scikit-learn/pull/22476
Skip to content
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

API Deprecate max_feature=auto for tree classes #22476

Merged

Conversation

MaxwellLZH
Copy link
Contributor

@MaxwellLZH MaxwellLZH commented Feb 14, 2022

Reference Issues/PRs

This is a fix to issue #22458 .

What does this implement/fix? Explain your changes.

This PR made the following changes:

  • Tree classes raise warnings when max_feature='auto'
  • Change ExtraTreeClassifier.max_feature default value to sqrt and ExtraTreeRegressor.max_feature default value to 1.0
  • Ignore deprecation warnings in _forest._parallel_build_trees function so we don't see repeated warning messages

sklearn/ensemble/_forest.py Outdated Show resolved Hide resolved
@glemaitre
Copy link
Contributor

@glemaitre glemaitre commented Feb 14, 2022

You will need to add some tests to check the behaviour of the warning and catch the warnings in all tests that are expected to raise the warning.

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Thanks for the PR!

sklearn/ensemble/_forest.py Outdated Show resolved Hide resolved
sklearn/tree/_classes.py Outdated Show resolved Hide resolved
sklearn/tree/_classes.py Outdated Show resolved Hide resolved
sklearn/tree/_classes.py Outdated Show resolved Hide resolved
sklearn/tree/_classes.py Outdated Show resolved Hide resolved
MaxwellLZH and others added 5 commits Feb 16, 2022
Co-authored-by: Thomas J. Fan <[email protected]>
Co-authored-by: Thomas J. Fan <[email protected]>
Co-authored-by: Thomas J. Fan <[email protected]>
Co-authored-by: Thomas J. Fan <[email protected]>
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Thanks for the update. The CI error comes from us during UserWarnings into errors. We likely need to add a filter warning on some test fails to filter out warning. Something like this:

pytestmark = pytest.mark.filterwarnings(

sklearn/ensemble/_base.py Outdated Show resolved Hide resolved
sklearn/tree/tests/test_tree.py Outdated Show resolved Hide resolved
sklearn/tree/tests/test_tree.py Outdated Show resolved Hide resolved
@MaxwellLZH
Copy link
Contributor Author

@MaxwellLZH MaxwellLZH commented Feb 17, 2022

We likely need to add a filter warning on some test fails to filter out warning

Hi @thomasjpfan , it seems like the failed test cases are scattered in a lot of different files due to the wide usage of tree models.

I'm wondering if there's a way to ignore that specific warning by using a global configuration during the CI process or we need to add filterwarnings individually in all of those files.

@thomasjpfan
Copy link
Member

@thomasjpfan thomasjpfan commented Feb 17, 2022

There looks to be only 4 files with warnings. For the following files, I suggest to update max_features to not use auto:

  • sklearn/ensemble/tests/test_gradient_boosting.py
  • sklearn/inspection/tests/test_partial_dependence.py
  • sklearn/tests/test_docstring_parameters.py

For sklearn/tree/tests/test_tree.py do a pytest.mark.filterwarnings on the file.

@MaxwellLZH
Copy link
Contributor Author

@MaxwellLZH MaxwellLZH commented Feb 19, 2022

I just figured that I forgot to change the default value in the __init__ function for ExtraTreeClassifier and ExtraTreeRegressor. Now it's updated.

Copy link
Member

@thomasjpfan thomasjpfan left a comment

Please add an entry to the change log at doc/whats_new/v1.1.rst with tag |API|. Like the other entries there, please reference this pull request with :pr: and credit yourself (and other contributors if applicable) with :user:.

sklearn/tree/_classes.py Show resolved Hide resolved
sklearn/tree/tests/test_tree.py Outdated Show resolved Hide resolved
sklearn/ensemble/tests/test_gradient_boosting.py Outdated Show resolved Hide resolved
Copy link
Member

@thomasjpfan thomasjpfan left a comment

Minor comment on the whats new. Otherwise LGTM

doc/whats_new/v1.1.rst Outdated Show resolved Hide resolved
@jeremiedbb jeremiedbb added this to the 1.1 milestone Mar 2, 2022
@jeremiedbb jeremiedbb assigned thomasjpfan and unassigned thomasjpfan Mar 2, 2022
Copy link
Member

@jeremiedbb jeremiedbb left a comment

Thanks @MaxwellLZH. Just a few nitpicks to help the cleaning in 1.3 and I think the deprecation warning test can be simplified.

sklearn/ensemble/_base.py Outdated Show resolved Hide resolved
sklearn/ensemble/tests/test_gradient_boosting.py Outdated Show resolved Hide resolved
sklearn/tree/tests/test_tree.py Outdated Show resolved Hide resolved
sklearn/tree/tests/test_tree.py Outdated Show resolved Hide resolved
sklearn/tree/tests/test_tree.py Outdated Show resolved Hide resolved
sklearn/tree/tests/test_tree.py Outdated Show resolved Hide resolved
sklearn/tree/tests/test_tree.py Outdated Show resolved Hide resolved
MaxwellLZH and others added 8 commits Mar 23, 2022
Co-authored-by: Jérémie du Boisberranger <[email protected]>
Co-authored-by: Jérémie du Boisberranger <[email protected]>
Co-authored-by: Jérémie du Boisberranger <[email protected]>
Co-authored-by: Jérémie du Boisberranger <[email protected]>
Co-authored-by: Jérémie du Boisberranger <[email protected]>
Co-authored-by: Jérémie du Boisberranger <[email protected]>
Co-authored-by: Jérémie du Boisberranger <[email protected]>
Copy link
Member

@jeremiedbb jeremiedbb left a comment

LGTM. Thanks @MaxwellLZH !

@jeremiedbb jeremiedbb merged commit e5736af into scikit-learn:main Mar 23, 2022
22 checks passed
glemaitre pushed a commit to glemaitre/scikit-learn that referenced this issue Apr 6, 2022
Co-authored-by: Thomas J. Fan <[email protected]>
Co-authored-by: Jérémie du Boisberranger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants