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
add feature_names_in_ and n_features_in_ attributes to dummy estimators #27937
add feature_names_in_ and n_features_in_ attributes to dummy estimators #27937
Conversation
The PR fails on a single test because the error type is now different ( def test_quantile_strategy_empty_train():
est = DummyRegressor(strategy="quantile", quantile=0.4)
with pytest.raises(ValueError):
est.fit([], []) Not sure if I can just change the error type in the test because I am unsure what the test is actually testing (that it fails or that a specific error is raised). |
I think it'd make sense to fix the test, the behavior is now more consistent with the other estimators. It would need a changelog entry though. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some nitpicks.
@yuanx749 can you review again? |
I just came across this PR and think it is good. But I am not a maintainer, so you need to ask a maintainer to approve. Sorry for the confusion... |
I see. @adrinjalali can you review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a test to make sure the attributes are correctly added to the estimator instance. Otherwise LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Otherwise LGTM.
@adrinjalali can this be merged? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for the PR @tvdboom ! LGTM
Reference Issues/PRs
Fixes #27907
What does this implement/fix? Explain your changes.
Add the attributes
feature_names_in_
andn_features_in_
to the Dummy estimators.