Skip to content
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

Fix SP creation to use correct # of attempts #817

Merged
merged 1 commit into from Sep 14, 2015
Merged

Conversation

pblouw
Copy link
Contributor

@pblouw pblouw commented Aug 20, 2015

Fixes a minor bug in the create_pointer method of the Vocabulary class that results in 100 attempts being made to generate an acceptable semantic pointer regardless of the number of attempts specified by the user.

@@ -92,7 +92,7 @@ def create_pointer(self, attempts=100, unitary=False):
count = 0
p = pointer.SemanticPointer(self.dimensions, rng=self.rng)
if self.vectors.shape[0] > 0:
while count < 100:
while count < attempts:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Damnit! That bug has been in the code for years. Good find!

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did anyone ever specify a different number of attempts? 😛

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not that I know of.

Oh, and while I'm thinking about it, every now and then I realize that this attempt thing really should keep track of the best one that it finds, and use that if it can't find one smaller than max_similarity. In other words, it could at least do the best it can, rather than just sticking with the last one if it fails.....

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I've updated this pull request with a new commit that includes Terry's suggestion. I checked that the best pointer is saved properly using a few print statements, but feel free to pass along any suggestions for improvements.

@pblouw
Copy link
Contributor Author

pblouw commented Aug 20, 2015

I suppose if you're trying keep the dimensionality of your SP's low for computational efficiency but want to use a relatively large number of them, it might be useful to increase the number of attempts. +1 for tracking the using the best candidate SP if the max_similarity constraint is not satisfied though.

@hunse
Copy link
Collaborator

hunse commented Aug 31, 2015

I fixed up the create_pointer function to be a little simpler. I also added a test for the warning. Looks great!

@tbekolay tbekolay added this to the 2.1.0 release milestone Aug 31, 2015
@tbekolay tbekolay added the bug label Aug 31, 2015
@tbekolay
Copy link
Member

Agreed, looks good! Should this get a changelog entry?

@hunse
Copy link
Collaborator

hunse commented Aug 31, 2015

Hmm, I guess it did fix a bug and change behaviour a little. Probably best to get in the habit of adding them, even for little things like this.

@hunse
Copy link
Collaborator

hunse commented Aug 31, 2015

Okay, added the changelog entry. Travis has gone INSANE!

@tbekolay
Copy link
Member

Travis has gone INSANE!

I believe that's because we're depending on IPython[notebook] which now no longer gives you the IPython notebok. You have to pip install jupyter. #693 fixes this, which I'm merging shortly, so should be no worries.

@hunse
Copy link
Collaborator

hunse commented Sep 14, 2015

@tbekolay, do you want to merge this one, since I re-wrote a good chunk of it?

@tbekolay
Copy link
Member

Sounds good, will look at this now.

Also, if a pointer cannot be found within the tolerance in the
specified number of attempts, return the best candidate SP.
@tbekolay tbekolay merged commit 608077c into master Sep 14, 2015
@tbekolay tbekolay deleted the vocab-fixes branch September 14, 2015 20:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

None yet

6 participants