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

add prototype for sklearn integration #87

Merged
merged 13 commits into from
Aug 26, 2024

Conversation

SimonBlanke
Copy link
Owner

No description provided.

@SimonBlanke SimonBlanke linked an issue Aug 22, 2024 that may be closed by this pull request
@fkiraly
Copy link
Contributor

fkiraly commented Aug 23, 2024

Looks great! I would suggest to add the parametrize_with_checks testing right a the start, for "test driven development"

@SimonBlanke
Copy link
Owner Author

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_

@SimonBlanke
Copy link
Owner Author

Looks great! I would suggest to add the parametrize_with_checks testing right a the start, for "test driven development"

Test added in 9c0d56e

opt = RandomSearchOptimizer()


def test_fit():
Copy link
Contributor

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.

Copy link
Owner Author

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.

@fkiraly
Copy link
Contributor

fkiraly commented Aug 26, 2024

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_

Is the best_estimator_ supposed to be a blueprint (state as after __init__), or a fitted stimator (state as after fit)? Maybe it is the latter and that is the issue?

From a design perspective, the former might make more sense, but I would go with whatever is consistent with sklearn GridSearchCV.

Copy link
Contributor

@fkiraly fkiraly left a 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

@SimonBlanke
Copy link
Owner Author

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.

@SimonBlanke SimonBlanke marked this pull request as ready for review August 26, 2024 14:52
@SimonBlanke SimonBlanke merged commit 87dff2d into master Aug 26, 2024
9 of 18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[ENH] sklearn compatible tuning wrapper estimator
2 participants