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

API Change surrounding inner cv #146

Closed
samihamdan opened this issue Sep 21, 2021 · 1 comment
Closed

API Change surrounding inner cv #146

samihamdan opened this issue Sep 21, 2021 · 1 comment

Comments

@samihamdan
Copy link
Collaborator

I find the api for defining the inner cv no intuitive and would like to change it.

Current API (complete example later):

model_params = model_params=dict(
    svm__C = [1,2,3],
    cv = StratifiedKFold(3),
    search='grid',
    search_params = dict(verbose=1)
)

My Problem

As you can see we specify arguments of the search inside of search_params.
But the argument cv is set outside of seach_params in model_params.
This can and did lead to confusion for users, as cv is a param of search,
but if you set it inside of search_params it there will neither result in the correct output nor a warning or error.

Example of the Problem:

image

Reproducible example of current API

import seaborn as sns 
from sklearn.model_selection import StratifiedKFold

from julearn import run_cross_validation
from julearn.utils import configure_logging
configure_logging(level='INFO')

df_iris = (sns.load_dataset('iris')
           .iloc[:100,:])
X = df_iris.columns[:-1].tolist()
y = 'species'

df_iris = sns.load_dataset('iris')
df_iris = df_iris.iloc[:100,:]

model_params = model_params=dict(
    svm__C = [1,2,3],
    cv = StratifiedKFold(3),
    search='grid',
    search_params = dict(verbose=1)
)
run_cross_validation(
    X=X , y=y, data=df_iris,
    model='svm', cv=3, 
    model_params=model_params
)

can be adjusted to also show the above mentioned strange behavior by changing model_params:

model_params = model_params=dict(
    svm__C = [1,2,3],
    search='grid',
    search_params = dict(
        cv = StratifiedKFold(3), verbose=1
    )

Proposed Changes:

I would propose that we only allow setting inner cv as cv in search_params.

@fraimondo
Copy link
Contributor

+1

Maybe do it as part of #145 ?

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

No branches or pull requests

2 participants