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.

Sign up for GitHub

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

Jump to bottom

SequentialFeatureSelection Early Stopping Criterion #886

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

SequentialFeatureSelection Early Stopping Criterion #886

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
8bedc45
adding early stop for sequential feature selection
aldder Feb 2, 2022
fff2163
test and refactoring
aldder Feb 2, 2022
f99fec4
improvements: https://github.com/rasbt/mlxtend/pull/886#issuecomment-…
aldder Feb 2, 2022
4db4c97
refactoring
aldder Feb 2, 2022
d5595a9
fix https://github.com/rasbt/mlxtend/pull/886#discussion_r797950840 a…
aldder Feb 3, 2022
c6e9ca8
fix https://github.com/rasbt/mlxtend/pull/886#discussion_r797954274
aldder Feb 7, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
refactoring
  • Loading branch information
@aldder
aldder committed Feb 2, 2022
commit 4db4c971cb2917dcd293a059f4ad12c4126ddef2
4 changes: 2 additions & 2 deletions mlxtend/feature_selection/sequential_feature_selector.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -567,8 +567,8 @@ def fit(self, X, y, custom_feature_names=None, groups=None, **fit_params):

# early stop
if self.early_stop_rounds \
and k != k_to_select \
and self.k_features in {'best', 'parsimonious'}:
and k != k_to_select \
and self.k_features in {'best', 'parsimonious'}:
Copy link
Owner

Choose a reason for hiding this comment

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

Instead of the check here, i would maybe change it to raising a ValueError in the top of the function if self.k_features is not in {'best', 'parsimonious'} and self.early_stop_rounds. This way the user is aware, and we only have to perform the check once.

Copy link
Author

@aldder aldder Feb 3, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

@rasbt Do you prefer having this check on top of fit function or during initialization?

Copy link
Owner

Choose a reason for hiding this comment

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

Ahh, totally lost track and missed your comment! Sorry! Regarding your question, I think fit might be better to keep it more consistent with scikit-learn behavior.

if k_score <= best_score:
early_stop_count -= 1
if early_stop_count == 0:
Expand Down