-
Notifications
You must be signed in to change notification settings - Fork 42
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 prototype for sklearn integration #87
Conversation
Looks great! I would suggest to add the |
Adding the following method (BestEstimator) leads to an AttributeError: "property 'n_features_in_' of 'HyperactiveSearchCV' object has no setter". Adding a setter does not solve the problem, so I will remove this method for now. @property
def n_features_in_(self):
try:
check_is_fitted(self)
except NotFittedError as nfe:
raise AttributeError(
"{} object has no n_features_in_ attribute.".format(
self.__class__.__name__
)
) from nfe
return self.best_estimator_.n_features_in_ |
Test added in 9c0d56e |
opt = RandomSearchOptimizer() | ||
|
||
|
||
def test_fit(): |
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.
these tests (this and below) feel a bit redundant, I think this should already be checked by parametrize_with_checks
, although I cannot tell for sure for all the things you are testing.
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.
The test_sklearn_api-file tests the methods and attributes of our public API. This might also include properties that are not covered by parametrize_with_checks
in the future.
Aside from that I am not opposed to some redundancy in my tests, since it can increase test-safety according to this model.
Is the From a design perspective, the former might make more sense, but I would go with whatever is consistent with |
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.
I would suggest to separate the CI modifications to a separate PR - just for cleanness, and eas of localizing errors if occurring.
Btw, if you squash PRs, no rebase or cherrypick magic is needed if you want to avoid that
I will keep that in mind in the future. |
No description provided.