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+1] Allows setting of initial hyperparameters for BayesianRidge #13618

Merged
merged 26 commits into from May 22, 2019

Conversation

c56pony
Copy link
Contributor

@c56pony c56pony commented Apr 11, 2019

Reference Issues/PRs

Fixes #13616

What does this implement/fix? Explain your changes.

Implements the fix for the problem suggested in the issue above.
Tests pass.

Any other comments?

@c56pony c56pony changed the title Allows setting of initial hyperparameters for BayesianRidge().fit [MRG] Allows setting of initial hyperparameters for BayesianRidge().fit Apr 12, 2019
Copy link
Member

@agramfort agramfort left a comment

You'll need to update a test and it would be great to expose this option by updating this example : https://scikit-learn.org/stable/auto_examples/linear_model/plot_bayesian_ridge.html

thanks @c56pony

sklearn/linear_model/bayes.py Outdated Show resolved Hide resolved
sklearn/linear_model/bayes.py Outdated Show resolved Hide resolved
Copy link
Contributor

@albertcthomas albertcthomas left a comment

I agree with @NicolasHug's comment in the issue, these parameters should be put in __init__. I would also add in the Notes section of the docstring that the initial values of the parameters can be set with alpha_init and lambda_init.

@c56pony c56pony changed the title [MRG] Allows setting of initial hyperparameters for BayesianRidge().fit [MRG] Allows setting of initial hyperparameters for BayesianRidge Apr 15, 2019
@c56pony
Copy link
Contributor Author

@c56pony c56pony commented Apr 15, 2019

Thank you for your comments @agramfort @albertcthomas @NicolasHug .

To-do list

  • Put new parameters in __init__
  • Change the name from alpha_0 to alpha_init
  • Change the name from lambda_0 to lambda_init
  • Update a test
  • Create a new example

@agramfort
Copy link
Member

@agramfort agramfort commented Apr 15, 2019

LGTM so far.

@c56pony
Copy link
Contributor Author

@c56pony c56pony commented Apr 15, 2019

Thanks @agramfort
I added new test and new example.

Copy link
Contributor

@albertcthomas albertcthomas left a comment

Thanks @c56pony. I would add a bit more details on the role of alpha_init and lambda_init so that it is a bit more transparent for the users.

  • In the Notes section of the Bayesian Ridge docstring, after "where updates of the regularization parameters are done as suggested in (MacKay, 1992).": The initial values of the update procedure can be be set with alpha_init and lambda_init.
  • Please update the User guide as the "remaining hyperparameters" now also includes alpha_init and lambda_init.

examples/linear_model/plot_bayesianridge_curvefit.py Outdated Show resolved Hide resolved
examples/linear_model/plot_bayesianridge_curvefit.py Outdated Show resolved Hide resolved
@c56pony
Copy link
Contributor Author

@c56pony c56pony commented Apr 17, 2019

Thanks for your advises, @agramfort @albertcthomas and @NicolasHug .

What I did

  1. Example
    • correct the __doc__
    • follow the sklearn standard naming conventions
    • use the random state object
    • use latex code for greek latters in the figure
  2. User guide
    • explain the role of alpha_init and lambda_init

Copy link
Contributor

@albertcthomas albertcthomas left a comment

Please also add a reference to the new example in the "Examples" section of the Bayesian Ridge regression user guide. Otherwise LGTM.

doc/modules/linear_model.rst Outdated Show resolved Hide resolved
@c56pony
Copy link
Contributor Author

@c56pony c56pony commented Apr 18, 2019

Thanks @albertcthomas .

As for adding "Examples" section to the User guide,
I don't know the link address generated automatically.
I guess "sphx_glr_auto_examples_linear_model_plot_bayesianridge_curvefit.py"
Is it correct?

@albertcthomas
Copy link
Contributor

@albertcthomas albertcthomas commented Apr 18, 2019

I guess "sphx_glr_auto_examples_linear_model_plot_bayesianridge_curvefit.py"
Is it correct?

Yes. Also it might be better to name the script plot_bayesian_ridge_curvefit.py for consistency with plot_bayesian_ridge.py

@c56pony
Copy link
Contributor Author

@c56pony c56pony commented Apr 19, 2019

fixed everything.
I also changed the name from plot_bayesianridge_curvefit.py to plot_bayesian_ridge_curvefit.py .

Copy link
Contributor

@albertcthomas albertcthomas left a comment

Rendered example. LGTM! Thanks @c56pony!

@c56pony
Copy link
Contributor Author

@c56pony c56pony commented Apr 19, 2019

Wow!
So excited because it is the first time for me to edit an example.

Copy link
Member

@agramfort agramfort left a comment

LGTM

can you just update the what's new page? thanks

@agramfort agramfort changed the title [MRG] Allows setting of initial hyperparameters for BayesianRidge [MRG+1] Allows setting of initial hyperparameters for BayesianRidge Apr 27, 2019
@c56pony
Copy link
Contributor Author

@c56pony c56pony commented May 6, 2019

I rebased to 0.22 and update the what's new. thanks

doc/whats_new/v0.22.rst Outdated Show resolved Hide resolved
doc/whats_new/v0.22.rst Outdated Show resolved Hide resolved
Copy link
Member

@jnothman jnothman left a comment

I forgot to submit these comments the other day. I've not yet completed the review

examples/linear_model/plot_bayesian_ridge_curvefit.py Outdated Show resolved Hide resolved
examples/linear_model/plot_bayesian_ridge_curvefit.py Outdated Show resolved Hide resolved
examples/linear_model/plot_bayesian_ridge_curvefit.py Outdated Show resolved Hide resolved
jnothman and others added 2 commits May 8, 2019
Co-Authored-By: c56pony <40843206+c56pony@users.noreply.github.com>
@c56pony
Copy link
Contributor Author

@c56pony c56pony commented May 20, 2019

Thank you @jnothman . I fixed everything.

Copy link
Member

@jnothman jnothman left a comment

only nitpicks

sklearn/linear_model/bayes.py Outdated Show resolved Hide resolved
sklearn/linear_model/bayes.py Outdated Show resolved Hide resolved
examples/linear_model/plot_bayesian_ridge_curvefit.py Outdated Show resolved Hide resolved
examples/linear_model/plot_bayesian_ridge_curvefit.py Outdated Show resolved Hide resolved
examples/linear_model/plot_bayesian_ridge_curvefit.py Outdated Show resolved Hide resolved
c56pony and others added 5 commits May 20, 2019
Co-Authored-By: Joel Nothman <joel.nothman@gmail.com>
Co-Authored-By: Joel Nothman <joel.nothman@gmail.com>
Co-Authored-By: Joel Nothman <joel.nothman@gmail.com>
Co-Authored-By: Joel Nothman <joel.nothman@gmail.com>
Co-Authored-By: Joel Nothman <joel.nothman@gmail.com>
@c56pony
Copy link
Contributor Author

@c56pony c56pony commented May 20, 2019

I've finished fixing them.

@jnothman
Copy link
Member

@jnothman jnothman commented May 22, 2019

Thanks @c56pony!

@jnothman jnothman merged commit 57c04f4 into scikit-learn:master May 22, 2019
14 of 16 checks passed
@c56pony c56pony deleted the work1 branch May 22, 2019
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

Successfully merging this pull request may close these issues.

5 participants