Skip to content

MAINT remove old code related to removed option max_features="auto"#27698

Merged
OmarManzoor merged 2 commits intoscikit-learn:mainfrom
glemaitre:doc_remove_max_features_auto
Nov 3, 2023
Merged

MAINT remove old code related to removed option max_features="auto"#27698
OmarManzoor merged 2 commits intoscikit-learn:mainfrom
glemaitre:doc_remove_max_features_auto

Conversation

@glemaitre
Copy link
Copy Markdown
Member

closes #27696

In scikit-learn 1.1, we deprecated max_features="auto" in decision trees and we remove this option in scikit-learn 1.3. However, we forgot to remove some of the occurrences from some docstring.

@github-actions
Copy link
Copy Markdown

github-actions bot commented Oct 31, 2023

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

Generated for commit: 17ad8da. Link to the linter CI: here

Copy link
Copy Markdown
Member

@lucyleeow lucyleeow left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

Now that parameter constraints will not allow max_features to be 'auto' we could probably delete these lines:

if self.max_features == "auto":
if is_classification:
max_features = max(1, int(np.sqrt(self.n_features_in_)))
warnings.warn(
(
"`max_features='auto'` has been deprecated in 1.1 "
"and will be removed in 1.3. To keep the past behaviour, "
"explicitly set `max_features='sqrt'`."
),
FutureWarning,
)
else:
max_features = self.n_features_in_
warnings.warn(
(
"`max_features='auto'` has been deprecated in 1.1 "
"and will be removed in 1.3. To keep the past behaviour, "
"explicitly set `max_features=1.0'`."
),
FutureWarning,
)

Comment on lines +1523 to +1524
`max(1, int(max_features * n_features_in_))` features are considered at
each split.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Does the indenting look off here? Should it align with 'If' in the line above?

@glemaitre glemaitre changed the title DOC remove 'auto' option for max_features MAINT remove old code related to removed option max_features="auto" Nov 1, 2023
@glemaitre
Copy link
Copy Markdown
Member Author

Indeed. We probably make some merging when the coverage was not working because this code is not covered anymore.

@lucyleeow lucyleeow added the Waiting for Second Reviewer First reviewer is done, need a second one! label Nov 2, 2023
Copy link
Copy Markdown
Contributor

@OmarManzoor OmarManzoor left a comment

Choose a reason for hiding this comment

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

LGTM

@OmarManzoor OmarManzoor merged commit 4ebc5e2 into scikit-learn:main Nov 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Documentation module:tree Waiting for Second Reviewer First reviewer is done, need a second one!

Projects

None yet

Development

Successfully merging this pull request may close these issues.

DecisionTreeClassifier does not support 'auto' as an option for max_features

3 participants