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

[MRG] implements log-uniform random variable #11232

Merged
merged 42 commits into from Oct 2, 2019

Conversation

@stsievert
Copy link
Contributor

stsievert commented Jun 11, 2018

Reference Issues/PRs

Fixes #9919, "implement/benchmark loguniform random variable for randomized search"

What does this implement/fix? Explain your changes.

This PR

  • implements a log-uniform random variable
  • provides tests
  • implements an example
  • is meant to be used with ParameterSampler, and implements a test accordingly

Any other comments?

This PR will be useful for dask/dask-searchcv#72

I believe I've followed everything in the contributor guidelines.

cc @sauln

@stsievert stsievert changed the title Lograndom [MRG] implements log-uniform random variable Jun 11, 2018
Copy link
Member

jnothman left a comment

Thanks for the PR. Tests are failing.

sklearn/utils/random.py Outdated Show resolved Hide resolved
examples/model_selection/plot_randomized_search.py Outdated Show resolved Hide resolved
@stsievert stsievert force-pushed the stsievert:lograndom branch from 875322b to f78fa34 Jun 12, 2018
@stsievert

This comment has been minimized.

Copy link
Contributor Author

stsievert commented Jun 12, 2018

Thanks for the review @jnothman. I've updated to address both comments. I return a scalar and have updated the example to use SGDClassifier instead of RandomForestClassifier.

examples/model_selection/plot_randomized_search.py Outdated Show resolved Hide resolved
sklearn/utils/random.py Outdated Show resolved Hide resolved
sklearn/utils/random.py Outdated Show resolved Hide resolved
sklearn/utils/random.py Outdated Show resolved Hide resolved
sklearn/utils/tests/test_random.py Outdated Show resolved Hide resolved
sklearn/utils/tests/test_random.py Outdated Show resolved Hide resolved
sklearn/utils/random.py Outdated Show resolved Hide resolved
sklearn/utils/tests/test_random.py Outdated Show resolved Hide resolved
@stsievert stsievert force-pushed the stsievert:lograndom branch from 0bd41d2 to 172920a Jun 13, 2018
@stsievert

This comment has been minimized.

Copy link
Contributor Author

stsievert commented Jun 13, 2018

Updated. Thanks for the review.

Copy link
Member

jnothman left a comment

Otherwise LGTM

sklearn/utils/tests/test_random.py Outdated Show resolved Hide resolved
sklearn/utils/random.py Outdated Show resolved Hide resolved
sklearn/utils/tests/test_random.py Outdated Show resolved Hide resolved
@jnothman

This comment has been minimized.

Copy link
Member

jnothman commented Jun 13, 2018

Please also list this in doc/modules/classes.rst. It's also worth mentioning in doc/modules/grid_search.rst

Please add an entry to the change log at doc/whats_new/v0.20.rst. Like the other entries there, please reference this pull request with :issue: and credit yourself (and other contributors if applicable) with :user:

@stsievert stsievert force-pushed the stsievert:lograndom branch 2 times, most recently from 0a9fb8e to 91d43d7 Jun 13, 2018
Copy link
Member

jnothman left a comment

Nice work

doc/whats_new/v0.20.rst Outdated Show resolved Hide resolved
sklearn/utils/random.py Outdated Show resolved Hide resolved
sklearn/utils/random.py Outdated Show resolved Hide resolved
sklearn/utils/random.py Outdated Show resolved Hide resolved
@stsievert stsievert mentioned this pull request Jun 21, 2018
7 of 7 tasks complete
@stsievert stsievert force-pushed the stsievert:lograndom branch from 8e75640 to f93a3e1 Jul 4, 2018
@stsievert

This comment has been minimized.

Copy link
Contributor Author

stsievert commented Jul 4, 2018

I've addressed the review, resolved merge conflicts by rebasing, and have tested locally on my machine with flake8.

doc/modules/grid_search.rst Outdated Show resolved Hide resolved
sklearn/utils/tests/test_random.py Outdated Show resolved Hide resolved
sklearn/utils/tests/test_random.py Outdated Show resolved Hide resolved
@stsievert

This comment has been minimized.

Copy link
Contributor Author

stsievert commented Jul 6, 2018

Updated.

As implemented, this API uses low, high arguments, which is the same as np.random.uniform. scipy.stats uses low, width arguments. I think we should mirror the API np.random.uniform uses because the names are so similar (loguniform vs uniform).

@jnothman

This comment has been minimized.

Copy link
Member

jnothman commented Feb 5, 2019

Please resolve conflicts with master, and maybe we can find a way towards review and merge.

@stsievert stsievert force-pushed the stsievert:lograndom branch from 19bffee to 6e685e6 Feb 5, 2019
stsievert added 2 commits Jun 11, 2018
MAINT: get loguniform working. proper API and example

TST: proper assertions on types

MAINT: do not silence warnings in example

MAINT: update for review

MAINT: add more review
MAINT: address review

MAINT: make edits

MAINT: circleci
@rth rth removed the Needs work label Sep 23, 2019
stsievert added 10 commits Sep 23, 2019
Copy link
Member

rth left a comment

A few more comments, otherwise looks good to me.

doc/whats_new/v0.22.rst Outdated Show resolved Hide resolved
doc/modules/classes.rst Outdated Show resolved Hide resolved
doc/modules/grid_search.rst Outdated Show resolved Hide resolved
doc/modules/grid_search.rst Outdated Show resolved Hide resolved
doc/modules/grid_search.rst Outdated Show resolved Hide resolved
examples/model_selection/plot_randomized_search.py Outdated Show resolved Hide resolved
examples/model_selection/plot_randomized_search.py Outdated Show resolved Hide resolved
sklearn/utils/fixes.py Show resolved Hide resolved
stsievert added 3 commits Sep 24, 2019
@rth
rth approved these changes Sep 24, 2019
sklearn/utils/fixes.py Outdated Show resolved Hide resolved
@glemaitre glemaitre self-requested a review Sep 24, 2019
@glemaitre

This comment has been minimized.

Copy link
Contributor

glemaitre commented Sep 24, 2019

Be aware that I made a push to trigger the build of the documentation.

@thomasjpfan

This comment has been minimized.

Copy link
Member

thomasjpfan commented Sep 24, 2019

Are we okay with supporting an alias?

@glemaitre

This comment has been minimized.

Copy link
Contributor

glemaitre commented Sep 24, 2019

I am fine with the atlas and once scipy/scipy#10815 is merged, we can import from scipy when available and later on deprecate it.

@rth

This comment has been minimized.

Copy link
Member

rth commented Sep 24, 2019

Are we okay with supporting an alias?

Once scipy/scipy#10815 is merged, it will be simply be a backport. However we can't backport code from that PR as that uses the private aliasing mechanism from scipy 1.3 as far as I understand. So this PR is still needed.

Copy link
Contributor

glemaitre left a comment

LGTM. I just fix something is in the doc and I will check the artefacts before to merge.

@glemaitre glemaitre merged commit 7cb5daf into scikit-learn:master Oct 2, 2019
19 checks passed
19 checks passed
LGTM analysis: C/C++ No code changes detected
Details
LGTM analysis: JavaScript No code changes detected
Details
LGTM analysis: Python No new or fixed alerts
Details
ci/circleci: deploy Your tests passed on CircleCI!
Details
ci/circleci: doc Your tests passed on CircleCI!
Details
ci/circleci: doc artifact Link to 0/doc/_changed.html
Details
ci/circleci: doc-min-dependencies Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 96.76%)
Details
codecov/project Absolute coverage decreased by -2.42% but relative coverage increased by +3.23% compared to 4e9f97d
Details
scikit-learn.scikit-learn Build #20191002.11 succeeded
Details
scikit-learn.scikit-learn (Linux py35_conda_openblas) Linux py35_conda_openblas succeeded
Details
scikit-learn.scikit-learn (Linux py35_ubuntu_atlas) Linux py35_ubuntu_atlas succeeded
Details
scikit-learn.scikit-learn (Linux pylatest_conda_mkl) Linux pylatest_conda_mkl succeeded
Details
scikit-learn.scikit-learn (Linux pylatest_pip_openblas_pandas) Linux pylatest_pip_openblas_pandas succeeded
Details
scikit-learn.scikit-learn (Linux32 py35_ubuntu_atlas_32bit) Linux32 py35_ubuntu_atlas_32bit succeeded
Details
scikit-learn.scikit-learn (Windows py35_pip_openblas_32bit) Windows py35_pip_openblas_32bit succeeded
Details
scikit-learn.scikit-learn (Windows py37_conda_mkl) Windows py37_conda_mkl succeeded
Details
scikit-learn.scikit-learn (macOS pylatest_conda_mkl) macOS pylatest_conda_mkl succeeded
Details
@stsievert stsievert deleted the stsievert:lograndom branch Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.