[MRG + 1] Return correct ridge parameter alpha_ and lambda_ for Bayesian ridge regression #8567
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
agramfort
reviewed
Mar 10, 2017
|
|
||
| # A Ridge regression model using an alpha value equal to the ratio of lambda_ and alpha_ from | ||
| # the Bayesian Ridge model must be identical | ||
| brModel = BayesianRidge(compute_score=True).fit(X, y) |
There was a problem hiding this comment.
we don't use CamelCase for var names.
brModel -> br or br_model
same for rrModel
agramfort
reviewed
Mar 10, 2017
| # the Bayesian Ridge model must be identical | ||
| brModel = BayesianRidge(compute_score=True).fit(X, y) | ||
| rrModel = Ridge(alpha=brModel.lambda_ / brModel.alpha_).fit(X, y) | ||
| assert_almost_equal(rrModel.intercept_, brModel.intercept_) |
There was a problem hiding this comment.
can you also test the coef_ ?
|
Implemented reviewer's suggestions:
|
Codecov Report
@@ Coverage Diff @@
## master #8567 +/- ##
==========================================
+ Coverage 95.48% 95.48% +<.01%
==========================================
Files 342 342
Lines 61013 61021 +8
==========================================
+ Hits 58259 58267 +8
Misses 2754 2754
Continue to review full report at Codecov.
|
raghavrv
suggested changes
Mar 13, 2017
| rr_model = Ridge(alpha=br_model.lambda_ / br_model.alpha_).fit(X, y) | ||
| assert_array_almost_equal(rr_model.coef_, br_model.coef_) | ||
| assert_almost_equal(rr_model.intercept_, br_model.intercept_) | ||
| # Results before fix |
|
And a whatsnew entry... |
|
after what's new entry and cleanup +1 for MRG |
|
Addressed requested changes:
|
raghavrv
approved these changes
Mar 13, 2017
|
thx @gedeck |
Closed
herilalaina
pushed a commit
to herilalaina/scikit-learn
that referenced
this pull request
Mar 26, 2017
…ian ridge regression (scikit-learn#8567) * Return correct ridge parameter alpha_ and lambda_ for regression * Add test for coefficients and fix style * Move sklearn.utils.testing to a more reasonable position. * Make flake8 happy * Code cleanup and entry in whats_new.rst
massich
pushed a commit
to massich/scikit-learn
that referenced
this pull request
Apr 26, 2017
…ian ridge regression (scikit-learn#8567) * Return correct ridge parameter alpha_ and lambda_ for regression * Add test for coefficients and fix style * Move sklearn.utils.testing to a more reasonable position. * Make flake8 happy * Code cleanup and entry in whats_new.rst
Sundrique
pushed a commit
to Sundrique/scikit-learn
that referenced
this pull request
Jun 14, 2017
…ian ridge regression (scikit-learn#8567) * Return correct ridge parameter alpha_ and lambda_ for regression * Add test for coefficients and fix style * Move sklearn.utils.testing to a more reasonable position. * Make flake8 happy * Code cleanup and entry in whats_new.rst
NelleV
pushed a commit
to NelleV/scikit-learn
that referenced
this pull request
Aug 11, 2017
…ian ridge regression (scikit-learn#8567) * Return correct ridge parameter alpha_ and lambda_ for regression * Add test for coefficients and fix style * Move sklearn.utils.testing to a more reasonable position. * Make flake8 happy * Code cleanup and entry in whats_new.rst
paulha
pushed a commit
to paulha/scikit-learn
that referenced
this pull request
Aug 19, 2017
…ian ridge regression (scikit-learn#8567) * Return correct ridge parameter alpha_ and lambda_ for regression * Add test for coefficients and fix style * Move sklearn.utils.testing to a more reasonable position. * Make flake8 happy * Code cleanup and entry in whats_new.rst
maskani-moh
pushed a commit
to maskani-moh/scikit-learn
that referenced
this pull request
Nov 15, 2017
…ian ridge regression (scikit-learn#8567) * Return correct ridge parameter alpha_ and lambda_ for regression * Add test for coefficients and fix style * Move sklearn.utils.testing to a more reasonable position. * Make flake8 happy * Code cleanup and entry in whats_new.rst
jwjohnson314
pushed a commit
to jwjohnson314/scikit-learn
that referenced
this pull request
Dec 18, 2017
…ian ridge regression (scikit-learn#8567) * Return correct ridge parameter alpha_ and lambda_ for regression * Add test for coefficients and fix style * Move sklearn.utils.testing to a more reasonable position. * Make flake8 happy * Code cleanup and entry in whats_new.rst
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Reference Issue
Fixes #8224
What does this implement/fix? Explain your changes.
In the original implementation, the
alpha_andlambda_were modified after calculation ofcoef_and were therefore slightly wrong. The change moves the assignment to the object attributesself.alpha_andself.lambda_to right after the calculation ofcoef_.Any other comments?
The test uses a comparison to the standard ridge regression. A ridge regression with a ridge parameter of
self.lambda_ / self.alpha_must return an identical regression. In the test, we compare the calculated intercept of the Bayesian ridge regression with the standard ridge regression. Before the change, the intercepts were slightly different.