Skip to content

add randomizedSearchCV example#14157

Merged
glemaitre merged 16 commits intoscikit-learn:masterfrom
yokasre:isaac-scikit
Jul 1, 2019
Merged

add randomizedSearchCV example#14157
glemaitre merged 16 commits intoscikit-learn:masterfrom
yokasre:isaac-scikit

Conversation

@yokasre
Copy link
Copy Markdown
Contributor

@yokasre yokasre commented Jun 22, 2019

Reference Issues/PRs

What does this implement/fix? Explain your changes.

The commit adds an example to the class sklearn.model_selection.RandomizedSearchCV

Any other comments?

@adrinjalali
Copy link
Copy Markdown
Member

related to #3846

the tests are failing

@yokasre
Copy link
Copy Markdown
Contributor Author

yokasre commented Jun 22, 2019

Okay. Re-running.

Copy link
Copy Markdown
Member

@thomasjpfan thomasjpfan left a comment

Choose a reason for hiding this comment

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

Please remove the checkpoint files and revert the change in .gitignore. Thank you for the PR!

@yokasre
Copy link
Copy Markdown
Contributor Author

yokasre commented Jun 22, 2019

Thanks @thomasjpfan
Let me fix that

@glemaitre
Copy link
Copy Markdown
Member

@Kasre96 You still have the .ipynb_checkpoint directory. You can remove it and make sure to not add it when doing git add.

Copy link
Copy Markdown
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution. This is super useful.

@yokasre
Copy link
Copy Markdown
Contributor Author

yokasre commented Jun 24, 2019

I've refined the example and I hope it passes.

>>> from scipy.stats import uniform
>>> iris = load_iris()
>>> logistic = LogisticRegression(max_iter=200)
>>> hyperparameters = dict(C=uniform(loc=0, scale=4),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
>>> hyperparameters = dict(C=uniform(loc=0, scale=4),
>>> hyperparameters = dict(C=rng.uniform(loc=0, scale=4),

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe name this distributions?

@yokasre
Copy link
Copy Markdown
Contributor Author

yokasre commented Jun 24, 2019

fixed @glemaitre
Thanks for this support

Copy link
Copy Markdown
Member

@glemaitre glemaitre left a comment

Choose a reason for hiding this comment

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

I just pushed a nitpick (reordering an import). I will merge it when it CIs are green.

@yokasre
Copy link
Copy Markdown
Contributor Author

yokasre commented Jun 25, 2019

Okay. Thank you.

@glemaitre glemaitre self-requested a review June 25, 2019 14:31
Copy link
Copy Markdown
Member

@jnothman jnothman left a comment

Choose a reason for hiding this comment

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

Otherwise LGTM

>>> from scipy.stats import uniform
>>> iris = load_iris()
>>> logistic = LogisticRegression(max_iter=200)
>>> hyperparameters = dict(C=uniform(loc=0, scale=4),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe name this distributions?

@glemaitre
Copy link
Copy Markdown
Member

Maybe name this distributions?

Are they distribution or just parameters? I find weird to call distrbutions when there is the penalty parameter.

@jnothman
Copy link
Copy Markdown
Member

It is a distribution (implied uniform) over penalties

@glemaitre
Copy link
Copy Markdown
Member

glemaitre commented Jun 26, 2019 via email

@rth
Copy link
Copy Markdown
Member

rth commented Jun 27, 2019

One last comment @Kasre96 #14157 (comment),

Maybe name this distributions?

otherwise this is ready to be merged. Thanks!

@yokasre
Copy link
Copy Markdown
Contributor Author

yokasre commented Jun 27, 2019

@rth I'm following through and noting all suggestions and changes. I'm sure they'll in effectiveness for my future PRs.
If all is well then I'm good and happy

@rth
Copy link
Copy Markdown
Member

rth commented Jun 28, 2019

I'm following through and noting all suggestions and changes

I meant that it would be necessary to address the above comment by jnothman for this PR to be merged. Thanks!

@glemaitre glemaitre merged commit 3aef9bd into scikit-learn:master Jul 1, 2019
@glemaitre
Copy link
Copy Markdown
Member

@Kasre96 I made the small changes and I merged. Thanks for your contribution!!!

@yokasre
Copy link
Copy Markdown
Contributor Author

yokasre commented Jul 1, 2019

Thanks @glemaitre
I'll keep working.

@yokasre yokasre deleted the isaac-scikit branch July 1, 2019 09:34
@yokasre yokasre restored the isaac-scikit branch July 1, 2019 09:35
koenvandevelde pushed a commit to koenvandevelde/scikit-learn that referenced this pull request Jul 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants