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

Allow integer vocabs for associative memories. #171

Merged
merged 1 commit into from
Jun 7, 2018
Merged

Conversation

jgosmann
Copy link
Collaborator

@jgosmann jgosmann commented Jun 4, 2018

Motivation and context:
For consistency, this was not working before. Not sure, if we should rather enforce an explicit vocabulary (with better error message) because only pointers already in the vocab at time of instantiation will be considered.

Interactions with other PRs:
none

How has this been tested?
added a test

How long should this take to review?

  • Quick (less than 40 lines changed or changes are straightforward)

Types of changes:

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have read the CONTRIBUTING.rst document.
  • I have updated the documentation accordingly.
  • I have included a changelog entry.
  • I have added tests to cover my changes.
  • I have run the test suite locally and all tests passed.

@jgosmann jgosmann requested a review from Seanny123 June 4, 2018 14:42
@jgosmann jgosmann added this to the 0.5.1 milestone Jun 4, 2018
@jgosmann
Copy link
Collaborator Author

jgosmann commented Jun 5, 2018

Despite what I discussed yesterday with @Seanny123, I am leaning toward this fix (instead of an exception) for consistency. While it might be easier to accidentally add additional pointers to a non-strict or default vocabulary, the same thing applies other vocabularies too. Maybe a warning if a non-strict vocabulary is used with an associative memory is better?

@jgosmann jgosmann self-assigned this Jun 5, 2018
@jgosmann jgosmann removed their assignment Jun 5, 2018
@jgosmann
Copy link
Collaborator Author

jgosmann commented Jun 6, 2018

I am wondering whether it might make sense to make to other changes:

  1. Allow sequences in addition to dictionaries as argument for the mapping parameter. A sequence would specify the keys used in an auto-associative (cleanup) memory. Given a non-strict vocab, the keys would be added. (Basically this would be syntactic sugar for {'A': 'A', 'B': 'B', ...}.)
  2. Require that mapping is set (i.e. no default value) to be explicit about keys to cleanup.

I think that would address the problem that pointers might be added to a vocabulary later as the user has to be explicit about the set of vectors cleaned up. It requires a bit more typing in simple cases: spa.ThresholdingAssocMem(0.3, vocab, mapping=vocab.keys()) or spa.ThresholdingAssocMem(0.3, d, mapping=model.vocabs[d].keys()).

@Seanny123
Copy link
Collaborator

I like the idea of making the mapping required. I think it's a level of explicitness that's useful. I'm not sure about letting sequences be passed as a parameter. I can't imagine it being misunderstood, but I guess I'm feeling pulled by Python's insistence of there only being one way to do things?

@jgosmann
Copy link
Collaborator Author

jgosmann commented Jun 6, 2018

I want to allow sequences if it is required because otherwise pure cleanup memories would require quite some boilerplate.

@Seanny123
Copy link
Collaborator

You're right and I've changed my mind. I'm fine with sequences as a manner to avoid annoying boilerplate.

@jgosmann
Copy link
Collaborator Author

jgosmann commented Jun 6, 2018

As that will change the API, I propose to merge this PR for a 0.5.1 bugfix release and have a separate PR for the further changes to be included in a 0.6 release.

@jgosmann jgosmann merged commit 82c32d3 into master Jun 7, 2018
@jgosmann jgosmann deleted the fix-int-d-assoc branch June 7, 2018 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants