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

Improve OptunaSearchCV (accept Mapping in addition to dict for param_distributions and allow use of OptunaSearchCV with cross_val_predict) #169

Closed
wants to merge 7 commits into from

Conversation

yu9824
Copy link
Contributor

@yu9824 yu9824 commented Oct 18, 2024

Motivation

I have two motivations.

  1. Accept Mapping in addition to dict for param_distributions.
  2. Allow use of OptunaSearchCV with cross_val_predict.

Description of the changes

1. Accept Mapping in addition to dict for param_distributions.

Changed the accepted type from dict to Mapping.

MappingProxyType and other types cannot be made to pickle, which causes an error during parallelization, so the specification has been changed to change to dict after accepting the Mapping type.

2. Allow use of OptunaSearchCV with cross_val_predict.

In scikit-learn's HasMethods, predict (and related methods) get error because of 'property', so define them as method.

@yu9824 yu9824 changed the title Improve OptunaSearchCV (accept Mapping except for dict for param_distributions and allow use of OptunaSearchCV with cross_val_predict) Improve OptunaSearchCV (accept Mapping in addition to dict for param_distributions and allow use of OptunaSearchCV with cross_val_predict) Oct 18, 2024
@y0z
Copy link
Member

y0z commented Oct 18, 2024

@nabenabe0928 Could you review this PR?

Copy link
Collaborator

@nabenabe0928 nabenabe0928 left a comment

Choose a reason for hiding this comment

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

Could you first move back each method to the original location?
Currently, this PR has much more diffs than we need.

Plus, could you give me the steps to reproduce your error?

@nabenabe0928
Copy link
Collaborator

@yu9824

So, I, first of all, found the exceptions where your changes do not work:

It means we cannot merge this PR as is, so I will close this PR for now.
However, please feel free to open this PR again if you find any other solutions!
Or you can alternatively post an issue for this regard with the steps to reproduce.

@yu9824
Copy link
Contributor Author

yu9824 commented Oct 22, 2024

Thank you for your review and comment.

I'll create issues alternatively to show you how to reproduce errors.
And, I'll deal with your comment.

@yu9824
Copy link
Contributor Author

yu9824 commented Oct 22, 2024

I've created new issues (#171, #173) and PRs (#172, #174).

I cannot reproduce your error below. Please tell me in the new PRs.

@yu9824

So, I, first of all, found the exceptions where your changes do not work:

It means we cannot merge this PR as is, so I will close this PR for now. However, please feel free to open this PR again if you find any other solutions! Or you can alternatively post an issue for this regard with the steps to reproduce.

I appreciate you taking the time to review this.

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.

3 participants