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

validation_curve and learning_curve should support fit_params #10252

Closed
jnothman opened this issue Dec 4, 2017 · 15 comments · Fixed by #18595
Closed

validation_curve and learning_curve should support fit_params #10252

jnothman opened this issue Dec 4, 2017 · 15 comments · Fixed by #18595
Labels
Easy Well-defined and straightforward way to resolve Enhancement good first issue Easy with clear instructions to resolve

Comments

@jnothman
Copy link
Member

jnothman commented Dec 4, 2017

Similar functions like cross_validation and cross_val_score support fit_params to pass sample_weight etc into the model being fitted. These other model selection routines should probably support the same.

@jnothman jnothman added Easy Well-defined and straightforward way to resolve Enhancement good first issue Easy with clear instructions to resolve help wanted labels Dec 4, 2017
@ido-sh
Copy link

ido-sh commented Dec 5, 2017

Hi @jnothman, can I give it a shot?

@jnothman
Copy link
Member Author

jnothman commented Dec 5, 2017

Sure, have a go. Remember to include or update a test

@ggc87
Copy link
Contributor

ggc87 commented Dec 5, 2017

Hi @jnothman,
I was checking this issue and I just saw that the tests for the validation_curve are in test_learning_curve, which is testing the deprecated implementation in learning_curve.py.
Should this test moved in test_cross_validation where validation_curve is implemented now?

@jnothman
Copy link
Member Author

jnothman commented Dec 5, 2017 via email

@ggc87
Copy link
Contributor

ggc87 commented Dec 5, 2017

Right, I didn't see that the tests are in the model_selection/tests folder, I just saw the test_validation.py in the main test folder.

@gxyd
Copy link
Contributor

gxyd commented Dec 8, 2017

@ggc87 @ido-sh Is anyone here working on the issue? I have a pull request ready locally, but since you already asked to work on the issue, I'll wait if you still want to work on it.

@ido-sh
Copy link

ido-sh commented Dec 8, 2017

@gxyd if yours is ready then please feel free to go ahead

@ggc87
Copy link
Contributor

ggc87 commented Dec 11, 2017

@gxyd same here I was waiting for @ido-sh since he asked before.

@gxyd
Copy link
Contributor

gxyd commented Dec 11, 2017

I've close my PR.

@ggc87
Copy link
Contributor

ggc87 commented Dec 11, 2017

@gxyd I'm sorry this has been a bit confusing. You had no reason to close your PR, you can go ahead :)

@akshkr
Copy link

akshkr commented Dec 13, 2017

Can I take this?

@jnothman
Copy link
Member Author

jnothman commented Dec 13, 2017 via email

@uwaisiqbal
Copy link

Has this been implemented? I wanted to use this for something I'm currently working on.

@gxyd
Copy link
Contributor

gxyd commented Jan 16, 2018

See #10273 . I need to address a few comments before it becomes available.

@littlewine
Copy link

What is the status of this item?
In case I am not mistaken, the code is ready and awaiting to be merged in the master branch, right?

Can I use the code already by updating the code from here?

Thanks in advance for your answer and sorry if I am not supposed to post such questions here :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Easy Well-defined and straightforward way to resolve Enhancement good first issue Easy with clear instructions to resolve
Projects
None yet
8 participants