-
Notifications
You must be signed in to change notification settings - Fork 32
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
Conversation
'predict', 'decision_function', 'predict_log_proba' and 'predict_proba'
OptunaSearchCV
(accept Mapping
except for dict
for param_distributions
and allow use of OptunaSearchCV
with cross_val_predict
)OptunaSearchCV
(accept Mapping
in addition to dict
for param_distributions
and allow use of OptunaSearchCV
with cross_val_predict
)
@nabenabe0928 Could you review this PR? |
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.
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?
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. |
Thank you for your review and comment. I'll create issues alternatively to show you how to reproduce errors. |
I've created new issues (#171, #173) and PRs (#172, #174). I cannot reproduce your error below. Please tell me in the new PRs.
I appreciate you taking the time to review this. |
Motivation
I have two motivations.
Mapping
in addition todict
forparam_distributions
.OptunaSearchCV
withcross_val_predict
.Description of the changes
1. Accept
Mapping
in addition todict
forparam_distributions
.Changed the accepted type from
dict
toMapping
.MappingProxyType
and other types cannot be made to pickle, which causes an error during parallelization, so the specification has been changed to change todict
after accepting theMapping
type.2. Allow use of
OptunaSearchCV
withcross_val_predict
.In scikit-learn's
HasMethods
,predict
(and related methods) get error because of 'property', so define them as method.