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 Calculation of standard deviation of predictions in ARDRegression #10153

Merged
merged 12 commits into from Nov 22, 2017

Conversation

Projects
None yet
3 participants
@jdoepfert
Contributor

jdoepfert commented Nov 16, 2017

Reference Issues/PRs

Fixes #10128

What does this implement/fix? Explain your changes.

This resolves the issue that a ValueError is thrown upon calling predict() with return_std=True after fitting ARDRegression() with particular inputs X:

    X = np.array([[1, 0],
                  [0, 0]])
    y = np.array([0, 0])
    clf = ARDRegression(n_iter=1)
    clf.fit(X, y)
    clf.predict(X, return_std=True)
ValueError: shapes (2,1) and (2,2) not aligned: 1 (dim 1) != 2 (dim 0)

I think this PR actually resolves the following bug in the algorithm:

  1. In the algorithm's fitting iteration loop (L473), first sigma_ and coef_ are updated, using the parameter estimates from the previous iteration (e.g. keep_lambda)
  2. Then, the parameters are updated (e.g. keep_lambda is updated according to lambda_ < self.threshold_lambda)

At prediction stage, a kind of keep_lambda comparison is again performed, using self.lambda_ < self.threshold_lambda (L551), to adapt the shape of X to the shape of self.sigma_.
However, due to the algorithm's structure outlined above, this self.lambda_ < self.threshold_lambda is not identical to the keep_lambda used for calculating the sigma_, since keep_lambda and lambda_ are updated after sigma_. Therefore, in rare occasions, when keep_lambda changes during the last iteration of the algorithm, the shapes of the adapted X and self.sigma_ in L552 will not match.

The fix consists of applying the updates for sigma_ and coef_ one more time after the last iteration of the algorithm. This way, the updated parameter estimates are used to calculate the final sigma_ and coef_. This is also in line with this publication, see discussion in #10128.

Show outdated Hide outdated sklearn/linear_model/tests/test_bayes.py Outdated
Show outdated Hide outdated sklearn/linear_model/tests/test_bayes.py Outdated
Show outdated Hide outdated sklearn/linear_model/tests/test_bayes.py Outdated
for iter_ in range(self.n_iter):
# Compute mu and sigma (using Woodbury matrix identity)
# Compute sigma and mu (using Woodbury matrix identity)
def update_sigma(X, alpha_, lambda_, keep_lambda, n_samples):

This comment has been minimized.

@glemaitre

glemaitre Nov 17, 2017

Contributor

i am not sure what is the best: keep the function here or make a private method.
@jnothman WDYT?

@glemaitre

glemaitre Nov 17, 2017

Contributor

i am not sure what is the best: keep the function here or make a private method.
@jnothman WDYT?

This comment has been minimized.

@agramfort

agramfort Nov 20, 2017

Member

I am fine leaving it here if the object still pickles

@agramfort

agramfort Nov 20, 2017

Member

I am fine leaving it here if the object still pickles

This comment has been minimized.

@jdoepfert

jdoepfert Nov 20, 2017

Contributor

Is this tested somewhere?

@jdoepfert

jdoepfert Nov 20, 2017

Contributor

Is this tested somewhere?

@codecov

This comment has been minimized.

Show comment
Hide comment
@codecov

codecov bot Nov 17, 2017

Codecov Report

Merging #10153 into master will decrease coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10153      +/-   ##
==========================================
- Coverage   96.21%   96.13%   -0.08%     
==========================================
  Files         337      337              
  Lines       62891    62942      +51     
==========================================
  Hits        60508    60508              
- Misses       2383     2434      +51
Impacted Files Coverage Δ
sklearn/linear_model/bayes.py 95.67% <100%> (+0.22%) ⬆️
sklearn/linear_model/tests/test_bayes.py 91.66% <100%> (+1.24%) ⬆️
sklearn/svm/bounds.py 94.73% <0%> (-5.27%) ⬇️
sklearn/model_selection/tests/test_split.py 92.29% <0%> (-4.15%) ⬇️
sklearn/tests/test_cross_validation.py 97.76% <0%> (-1.97%) ⬇️
sklearn/utils/testing.py 76.48% <0%> (-0.52%) ⬇️
sklearn/model_selection/_validation.py 96.91% <0%> (-0.02%) ⬇️
sklearn/cross_validation.py 98.59% <0%> (-0.01%) ⬇️
sklearn/model_selection/_split.py 99.13% <0%> (-0.01%) ⬇️
sklearn/linear_model/tests/test_omp.py 100% <0%> (ø) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5963fd2...7741afc. Read the comment docs.

codecov bot commented Nov 17, 2017

Codecov Report

Merging #10153 into master will decrease coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #10153      +/-   ##
==========================================
- Coverage   96.21%   96.13%   -0.08%     
==========================================
  Files         337      337              
  Lines       62891    62942      +51     
==========================================
  Hits        60508    60508              
- Misses       2383     2434      +51
Impacted Files Coverage Δ
sklearn/linear_model/bayes.py 95.67% <100%> (+0.22%) ⬆️
sklearn/linear_model/tests/test_bayes.py 91.66% <100%> (+1.24%) ⬆️
sklearn/svm/bounds.py 94.73% <0%> (-5.27%) ⬇️
sklearn/model_selection/tests/test_split.py 92.29% <0%> (-4.15%) ⬇️
sklearn/tests/test_cross_validation.py 97.76% <0%> (-1.97%) ⬇️
sklearn/utils/testing.py 76.48% <0%> (-0.52%) ⬇️
sklearn/model_selection/_validation.py 96.91% <0%> (-0.02%) ⬇️
sklearn/cross_validation.py 98.59% <0%> (-0.01%) ⬇️
sklearn/model_selection/_split.py 99.13% <0%> (-0.01%) ⬇️
sklearn/linear_model/tests/test_omp.py 100% <0%> (ø) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5963fd2...7741afc. Read the comment docs.

@jdoepfert

This comment has been minimized.

Show comment
Hide comment
@jdoepfert

jdoepfert Nov 17, 2017

Contributor

Any idea why appveyor fails? def test_predict_proba_binary() in sklearn/neural_network/tests/test_mlp.py fails, if I interpret the huge log correctly. On my machine, the tests run without problems.

EDIT: appveyor now passes again

Contributor

jdoepfert commented Nov 17, 2017

Any idea why appveyor fails? def test_predict_proba_binary() in sklearn/neural_network/tests/test_mlp.py fails, if I interpret the huge log correctly. On my machine, the tests run without problems.

EDIT: appveyor now passes again

@jdoepfert jdoepfert changed the title from [WIP] Fix bug in ARDRegression: Calculation of standard deviation of predictions to [MRG] Fix bug in ARDRegression: Calculation of standard deviation of predictions Nov 18, 2017

@glemaitre

This comment has been minimized.

Show comment
Hide comment
@glemaitre

glemaitre Nov 18, 2017

Contributor

LGTM

ping @agramfort @jnothman (you might want to give your opinion regarding private methods/locally defined function)

Contributor

glemaitre commented Nov 18, 2017

LGTM

ping @agramfort @jnothman (you might want to give your opinion regarding private methods/locally defined function)

@glemaitre glemaitre changed the title from [MRG] Fix bug in ARDRegression: Calculation of standard deviation of predictions to [MRG + 1] FIX Calculation of standard deviation of predictions in ARDRegression Nov 18, 2017

@agramfort

This comment has been minimized.

Show comment
Hide comment
@agramfort

agramfort Nov 20, 2017

Member

@jdoepfert please document the bug fix in what's new page.

Member

agramfort commented Nov 20, 2017

@jdoepfert please document the bug fix in what's new page.

jdoepfert added some commits Nov 21, 2017

Merge branch 'master' into fix/10128 and resolve conflicts
# Conflicts:
#	doc/whats_new/v0.20.rst
@jdoepfert

This comment has been minimized.

Show comment
Hide comment
@jdoepfert

jdoepfert Nov 21, 2017

Contributor

@agramfort I updated what's new. Hopefully correctly, I am not sure about the format.

Contributor

jdoepfert commented Nov 21, 2017

@agramfort I updated what's new. Hopefully correctly, I am not sure about the format.

@glemaitre glemaitre merged commit 7ffad2c into scikit-learn:master Nov 22, 2017

6 checks passed

ci/circleci Your tests passed on CircleCI!
Details
codecov/patch 100% of diff hit (target 96.1%)
Details
codecov/project 96.1% (+<.01%) compared to 8e1efb0
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
@glemaitre

This comment has been minimized.

Show comment
Hide comment
@glemaitre

glemaitre Nov 22, 2017

Contributor

Ok so that's a MRG+2. thanks @jdoepfert

Contributor

glemaitre commented Nov 22, 2017

Ok so that's a MRG+2. thanks @jdoepfert

@jdoepfert

This comment has been minimized.

Show comment
Hide comment
@jdoepfert

jdoepfert Nov 22, 2017

Contributor

@glemaitre thx for reviewing!

Contributor

jdoepfert commented Nov 22, 2017

@glemaitre thx for reviewing!

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment