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] Fixes n_iter_without_progress and min_grad_norm in TSNE #6497

Closed
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@ssaeger
Contributor

ssaeger commented Mar 6, 2016

Fixes #6450.
The parameters n_iter_without_progress and min_grad_norm are now correctly used and a test was added to ensure that n_iter_without_progress is used correctly.

@AlexanderFabisch

This comment has been minimized.

Member

AlexanderFabisch commented Mar 21, 2016

Seems to be ok for me. Could you add a test for min_grad_norm as well? The build errors should be unrelated!?

@ssaeger

This comment has been minimized.

Contributor

ssaeger commented Mar 21, 2016

The build errors are not related. I will see how I can test min_grad_norm, maybe I have already an idea. :)

@ssaeger

This comment has been minimized.

Contributor

ssaeger commented Mar 21, 2016

Ok, I added a test for min_grad_norm.
It checks if the gradient norm values in the verbose output are smaller than min_grad_norm at most once, because if the gradient norm becomes smaller the optimization stops.

@colinmorris

This comment has been minimized.

colinmorris commented Sep 15, 2016

Sorry to butt in, but I'm wondering if this P.R. slipped through the cracks, or if there's another reason it hasn't been merged? I just got hit by #6450 myself, and it's pretty hard to work around.

@jnothman

This comment has been minimized.

Member

jnothman commented Sep 15, 2016

Yes, slipped through the cracks. Thanks for resurrecting it.

@jnothman jnothman added the Bug label Sep 15, 2016

# The gradient norm can be smaller than min_grad_norm at most once,
# because in the moment it becomes smaller the optimization stops
assert_less_equal(n_smaller_gradient_norms, 1)

This comment has been minimized.

@jnothman

jnothman Sep 15, 2016

Member

Needs newline at end of file.

@jnothman

This comment has been minimized.

Member

jnothman commented Sep 15, 2016

I'm not loving the text hacking, but as a fix I think this is good.

@jnothman jnothman changed the title from [MRG] Fixes n_iter_without_progress and min_grad_norm in TSNE to [MRG+1] Fixes n_iter_without_progress and min_grad_norm in TSNE Sep 15, 2016

@jnothman

This comment has been minimized.

Member

jnothman commented Sep 15, 2016

@AlexanderFabisch, could you please review the test for min_grad_norm?

sys.stdout = old_stdout
# The output needs to contain the value of n_iter_without_progress
assert("did not make any progress during the "

This comment has been minimized.

@AlexanderFabisch

AlexanderFabisch Sep 16, 2016

Member

You can use nose.tools.assert_in

@amueller

This comment has been minimized.

Member

amueller commented Oct 10, 2016

@colinmorris did this PR fix your issue?

@amueller amueller added this to the 0.18.1 milestone Oct 10, 2016

@amueller

Looks good to me. Should we merge?

Maximum number of iterations without progress before we abort the
optimization.
optimization. If method='barnes_hut' this parameter is fixed to

This comment has been minimized.

@amueller

amueller Oct 10, 2016

Member

This behavior is somewhat peculiar and we could work around it, but let's leave it for now.

@AlexanderFabisch

This comment has been minimized.

Member

AlexanderFabisch commented Oct 11, 2016

It fixes a bug. So we should merge it. Maybe we should add the newline at the end of the test file.

@jnothman

This comment has been minimized.

Member

jnothman commented Oct 12, 2016

Merged as 2221387. @amueller, I've created a 0.18.1 what's new section, but have not backported this commit to the 0.18.X branch.

@jnothman jnothman closed this Oct 12, 2016

jnothman added a commit that referenced this pull request Oct 12, 2016

amueller added a commit to amueller/scikit-learn that referenced this pull request Oct 14, 2016

DOC what's new for scikit-learn#6497 and 0.18.1 section
# Conflicts:
#	doc/whats_new.rst

afiodorov added a commit to unravelin/scikit-learn that referenced this pull request Apr 25, 2017

Sundrique added a commit to Sundrique/scikit-learn that referenced this pull request Jun 14, 2017

paulha added a commit to paulha/scikit-learn that referenced this pull request Aug 19, 2017

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

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