Skip to content

[MRG] Document bounds='fixed' in GP kernels #16209

Merged
ogrisel merged 12 commits intoscikit-learn:masterfrom
bharathi-srini:doc_fix
Feb 21, 2020
Merged

[MRG] Document bounds='fixed' in GP kernels #16209
ogrisel merged 12 commits intoscikit-learn:masterfrom
bharathi-srini:doc_fix

Conversation

@bharathi-srini
Copy link
Copy Markdown

@bharathi-srini bharathi-srini commented Jan 25, 2020

Reference Issues/PRs

Fixes #8358. See also #11274.
See also PR #8391

What does this implement/fix? Explain your changes.

All bounds used in in GP kernels can be 'fixed'. If this option is used, the bounds will not change during hyperparameter tuning. This PR documents this in the docstrings of all the hyperparameters.

Comments

Worked with @hahey at the Berlin scikit-learn sprint 2020

jnothman
jnothman approved these changes Jan 26, 2020
View reviewed changes
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.

I like this.

@cmarmo
Copy link
Copy Markdown
Contributor

cmarmo commented Feb 4, 2020

@amueller @NicolasHug (from git blame suggestions), could you please take a look? Failed checks are unrelated with modifications. Thanks!

NicolasHug
NicolasHug approved these changes Feb 4, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@NicolasHug NicolasHug left a comment

Choose a reason for hiding this comment

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

Small style suggestions, please also apply them to the rest of the changes.

Otherwise LGTM, thanks @bharathi-srini

@cmarmo
Copy link
Copy Markdown
Contributor

cmarmo commented Feb 14, 2020

Hi @bharathi-srini this PR already has two approvals: do you think you could find some time to sync to master and apply the suggestions? Thanks for your work!

Bharathi Srinivasan and others added 4 commits February 16, 2020 21:07
@NicolasHug
Update sklearn/gaussian_process/kernels.py
ba48e18 
Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com>
@NicolasHug
Update sklearn/gaussian_process/kernels.py
fc868b4 
Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com>
@NicolasHug
Update sklearn/gaussian_process/kernels.py
00688d7 
Co-Authored-By: Nicolas Hug <contact@nicolas-hug.com>
@xBharathiSrini
updating other docstrings with recommended suggestions
93405e2
@bharathi-srini
Copy link
Copy Markdown
Author

bharathi-srini commented Feb 16, 2020

@cmarmo It is done :D Merge conflict needs to be resolved by a code owner though!

@cmarmo
Copy link
Copy Markdown
Contributor

cmarmo commented Feb 18, 2020

@bharathi-srini, what I meant is that you need to git fetch upstream and merge the upstream/master branch in your PR in order to resolve conflicts that arose because of new commits in the upstream repository of scikit-learn. Sorry if I misunderstood... maybe @elainejiang8 or @hahey could do that? Thanks!

@ogrisel
Copy link
Copy Markdown
Member

ogrisel commented Feb 20, 2020

@cmarmo It is done :D Merge conflict needs to be resolved by a code owner though!

You have the rights to merge master into your branch. If you need help or prefer us to do it for your let us know but it should not be very hard to do. You can try to use the "Resolve conflicts" button of github if you are not comfortable with doing it from the command line:

https://help.github.com/en/github/collaborating-with-issues-and-pull-requests/resolving-a-merge-conflict-on-github

@ogrisel
Copy link
Copy Markdown
Member

ogrisel commented Feb 21, 2020

Thanks @bharathi-srini. I just pushed two commits to remove some extra blank lines and restore the use of default= instead of default: that was lost at conflict resolution time.

The float > 0 idiom has not been fixed consistently in #16415 so I decided to leave it as such. We can always decide to fix it another PR but I am not sure it's worth it.

@ogrisel ogrisel merged commit 6464e15 into scikit-learn:master Feb 21, 2020
@ogrisel ogrisel mentioned this pull request Feb 21, 2020
Closed
thomasjpfan pushed a commit to thomasjpfan/scikit-learn that referenced this pull request Feb 22, 2020
@thomasjpfan
[MRG] Document bounds='fixed' in GP kernels (scikit-learn#16209)
05dcff3
panpiort8 pushed a commit to panpiort8/scikit-learn that referenced this pull request Mar 3, 2020
[MRG] Document bounds='fixed' in GP kernels (scikit-learn#16209)
69f404b
gio8tisu pushed a commit to gio8tisu/scikit-learn that referenced this pull request May 15, 2020
@gio8tisu
[MRG] Document bounds='fixed' in GP kernels (scikit-learn#16209)
48b25f7
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jnothman jnothman jnothman approved these changes

+1 more reviewer

@NicolasHug NicolasHug NicolasHug approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Document length_scale_bounds="fixed" in GP kernels.

6 participants

@bharathi-srini @cmarmo @ogrisel @jnothman @NicolasHug @xBharathiSrini