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] Bug fix for unnormalized laplacian #4995

Merged
merged 14 commits into from Oct 19, 2015

Conversation

Projects
None yet
5 participants
@yanlend
Contributor

yanlend commented Jul 17, 2015

So far, _set_diag() always set the diagonal of the Laplacian to 1. This is only valid for the normalized Laplacian. For the unnormalized Laplacian, the diagonal should not be changed.

Bug fix for unnormalized laplacian
So far, _set_diag() always set the diagonal of the Laplacian to 1. This is only valid for the normalized Laplacian. For the unnormalized Laplacian, the diagonal should not be changed.
@agramfort

This comment has been minimized.

Member

agramfort commented Jul 20, 2015

can you add a test?

@yanlend

This comment has been minimized.

Contributor

yanlend commented Jul 20, 2015

I added a test that checks w.r.t. manual dense eigendecomposition. I don't know if that's sufficient.

@amueller

This comment has been minimized.

Member

amueller commented Jul 29, 2015

LGTM

@amueller amueller changed the title from Bug fix for unnormalized laplacian to [MRG + 1] Bug fix for unnormalized laplacian Jul 29, 2015

@amueller

This comment has been minimized.

Member

amueller commented Jul 29, 2015

please add an entry to whatsnew.rst

yanlend added some commits Aug 6, 2015

Update whats_new.rst
added information for this bugfix
Merge branch 'master' of https://github.com/scikit-learn/scikit-learn
…into patch-1

Conflicts:
	doc/whats_new.rst
@yanlend

This comment has been minimized.

Contributor

yanlend commented Aug 6, 2015

I added the entry to whatsnew.rst and pulled all the changes from scikit-master, such that merging should be possible.

@amueller

This comment has been minimized.

Member

amueller commented Aug 10, 2015

Thanks. We need another review, though.

@amueller

This comment has been minimized.

Member

amueller commented Sep 9, 2015

ping @ogrisel.
@yanlend could you please rebase?

@amueller amueller added the Bug label Sep 9, 2015

yanlend added some commits Jul 17, 2015

Bug fix for unnormalized laplacian
So far, _set_diag() always set the diagonal of the Laplacian to 1. This is only valid for the normalized Laplacian. For the unnormalized Laplacian, the diagonal should not be changed.
Update whats_new.rst
added information for this bugfix
Merge branch 'patch-1' of https://github.com/yanlend/scikit-learn int…
…o patch-1

Conflicts:
	doc/whats_new.rst
Merge branch 'master' of https://github.com/scikit-learn/scikit-learn
…into patch-1

Conflicts:
	doc/whats_new.rst
@yanlend

This comment has been minimized.

Contributor

yanlend commented Oct 19, 2015

@amueller rebase is done.

@GaelVaroquaux

This comment has been minimized.

Member

GaelVaroquaux commented Oct 19, 2015

LGTM.

Merging without appveyor, as it is drowning from the sprint.

GaelVaroquaux added a commit that referenced this pull request Oct 19, 2015

Merge pull request #4995 from yanlend/patch-1
[MRG + 1] Bug fix for unnormalized laplacian

@GaelVaroquaux GaelVaroquaux merged commit 3ab597c into scikit-learn:master Oct 19, 2015

1 of 2 checks passed

continuous-integration/appveyor Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@yanlend yanlend deleted the yanlend:patch-1 branch Oct 19, 2015

@amueller

This comment has been minimized.

Member

amueller commented on 043dc9f Oct 19, 2015

@ogrisel this also backport? not sure...

This comment has been minimized.

Member

GaelVaroquaux replied Oct 19, 2015

@amueller amueller referenced this pull request Jan 20, 2016

Merged

[MRG] Joblib 0.9.4 #6179

@ogrisel

This comment has been minimized.

Member

ogrisel commented Jan 28, 2016

+1 for focusing on regressions.

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