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] Fix BayesianRidge() and ARDRegression() for constant target vectors #10095

Merged
merged 17 commits into from Nov 10, 2017

Conversation

Projects
None yet
4 participants
@jdoepfert
Contributor

jdoepfert commented Nov 9, 2017

Reference Issues/PRs

Fixes #10092

What does this implement/fix? Explain your changes.

This PR fixes the issue that when fitting a BayesianRidge or ARDRegression classifier with a constant target vector y, the predict method yields NaN arrays for both predictions and the respective standard devitions. This occurs since the estimated precision for the noise alpha_ is initialized with 1/np.var(y) = inf.

This PR fixes this issue by initializing alpha_ with 1/(np.var(y) + eps), where eps = np.spacing(1).

Any other comments?

The returned standard deviation std for the predictions will not be zero for a constant target vector (as suggested in #10092), but a small number instead. I added a test for this. However, std approaches zero as the number of samples increase, as expected:
image

@glemaitre

This comment has been minimized.

Show comment
Hide comment
@glemaitre

glemaitre Nov 9, 2017

Contributor

I would have thought that alpha_ = 1. / (np.var(y) + np.spacing(1)) should be enough

Contributor

glemaitre commented Nov 9, 2017

I would have thought that alpha_ = 1. / (np.var(y) + np.spacing(1)) should be enough

@jdoepfert jdoepfert changed the title from [MRG] Fix BayesianRidge() for constant target vectors to [WIP] Fix BayesianRidge() for constant target vectors Nov 9, 2017

@jdoepfert

This comment has been minimized.

Show comment
Hide comment
@jdoepfert

jdoepfert Nov 9, 2017

Contributor

@glemaitre Yea I like that solution better as well, will implement it. But note that then std will not be zero, as suggested in the issue.

Contributor

jdoepfert commented Nov 9, 2017

@glemaitre Yea I like that solution better as well, will implement it. But note that then std will not be zero, as suggested in the issue.

Show outdated Hide outdated sklearn/linear_model/bayes.py Outdated

@jdoepfert jdoepfert changed the title from [WIP] Fix BayesianRidge() for constant target vectors to [WIP] Fix BayesianRidge() and ARDRegression() for constant target vectors Nov 9, 2017

@jdoepfert jdoepfert changed the title from [WIP] Fix BayesianRidge() and ARDRegression() for constant target vectors to [MRG] Fix BayesianRidge() and ARDRegression() for constant target vectors Nov 9, 2017

@glemaitre glemaitre changed the title from [MRG] Fix BayesianRidge() and ARDRegression() for constant target vectors to [MRG + 1] Fix BayesianRidge() and ARDRegression() for constant target vectors Nov 10, 2017

@agramfort agramfort merged commit c6005e1 into scikit-learn:master Nov 10, 2017

6 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 96.19%)
Details
codecov/project 96.2% (+<.01%) compared to effd75d
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
lgtm analysis: Python No alert changes
Details
@agramfort

This comment has been minimized.

Show comment
Hide comment
@agramfort
Member

agramfort commented Nov 10, 2017

@jdoepfert

This comment has been minimized.

Show comment
Hide comment
@jdoepfert

jdoepfert Nov 11, 2017

Contributor

awesome!

Contributor

jdoepfert commented Nov 11, 2017

awesome!

maskani-moh added a commit to maskani-moh/scikit-learn that referenced this pull request Nov 15, 2017

[MRG + 1] Fix BayesianRidge() and ARDRegression() for constant target…
… vectors (scikit-learn#10095)

* add test for issue scikit-learn#10092

* add comment to test

* split into two tests

* add tests for scores, alpha and beta

* adapt tests: n_samples != n_features

* add test when no intercept is fitted

* add handling of constant target vector when intercept is fitted

* fix typo in comments

* fix format issues

* replace original fix with simpler fix

* add comment

* increase upper boundary for test

* increase upper boundary for test

* merge tests for ARDRegression and BayesianRidge

* use random state in tests

* decrease upper bound for std

* replace np.spacing(1) -> np.finfo(np.float64).eps

jwjohnson314 pushed a commit to jwjohnson314/scikit-learn that referenced this pull request Dec 18, 2017

[MRG + 1] Fix BayesianRidge() and ARDRegression() for constant target…
… vectors (scikit-learn#10095)

* add test for issue scikit-learn#10092

* add comment to test

* split into two tests

* add tests for scores, alpha and beta

* adapt tests: n_samples != n_features

* add test when no intercept is fitted

* add handling of constant target vector when intercept is fitted

* fix typo in comments

* fix format issues

* replace original fix with simpler fix

* add comment

* increase upper boundary for test

* increase upper boundary for test

* merge tests for ARDRegression and BayesianRidge

* use random state in tests

* decrease upper bound for std

* replace np.spacing(1) -> np.finfo(np.float64).eps
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment