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.

Sign up for GitHub

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

Jump to bottom

Fixes #1903 - Re-use Connections class in SP #2066

Closed
wants to merge 3 commits into from
Closed

Fixes #1903 - Re-use Connections class in SP #2066

wants to merge 3 commits into from

Conversation

david-ragazzi
Copy link
Contributor

Fixes: #1903
Depends: #1449

Ladies and gentlemen, now SP also has connections! Connections class is a prime by @chetan51 which allow us to separate net specification from net state. This seems simple but has a lot of implications to make nupic easier to understanding and mantaining. This class is so abstract when comes to segments and synapses that I managed adapt it to SP without need create any additional method but only re-using the existing ones.

Some important changes:

  • _potentialPools was removed: Now SP create synapses to all input bits that are in potential area of a column. So when methods like _adaptSynapses, _bumpUpWeakColumns, _updatePermanencesForColumn, need update synapses's permanencies, they don't need handle a mapPotential but simply handle directly such synapses of the column or get the indices of the input bits which they are connected.
  • _permanences was removed: Now SP uses a dict to get permanencies from an individual column and put them into a temporary numpy array to perform changes (perm). After permanencies operations is done, SP updates the synapses of a column with the new permanencies values (using _updatePermanencesForColumn method).

I compared the performance without or with these changes and I feel litle difference. But it is desirable we have more eyes on this.

This PR still is not ready because I still have to implement serialization for Connections and more tests are need to we find areas for improvement.

TODO:

  • Move Connections class from TemporalMemory into a separated file.
  • Adapt SP to use Connections.
  • Create serialization for Connections.
  • Compare performance.

@david-ragazzi david-ragazzi self-assigned this Apr 20, 2015
@david-ragazzi david-ragazzi force-pushed the use_connections_in_SP branch from d23c608 to 5c46aab Compare April 20, 2015 19:58
@breznak
Copy link
Member

breznak commented Apr 21, 2015

wow 🆒 A really exciting change! I think I'll need a brain for the review, so I'll leave that for tmr, but it looks really 👍 👍

@david-ragazzi
Copy link
Contributor Author

@breznak haha.. Really this won't an easy job, I even had took advantage that I was with fresh brain and without distractions to finally work on this.. If this improvement has similar or better performance (who knows), then we should say goodbye to the sparse matrix.. and so finally SP will be more intuitive to understand.. :-D

Talking about performance, in the current state of this PR, SP has to transverse synapses to get the input indices which they are connected at every time that such indices are need. I'm thinking of use dicts for perform this operation once and so gain performance.. I'm open to suggestions..

@breznak
Copy link
Member

breznak commented Apr 21, 2015

I think this would also fix this old pal, #1385
because the methods are not used at all after this PR

@breznak
Copy link
Member

breznak commented Apr 21, 2015

I've just cleaned up the python benchmark PR, so you can use #1928 to measure the new SP performance.

@david-ragazzi david-ragazzi force-pushed the use_connections_in_SP branch from 5c46aab to b8b228e Compare April 22, 2015 18:27
breznak
breznak reviewed Apr 24, 2015
View reviewed changes
nupic/research/spatial_pooler.py
synapses = self.connections.synapsesForSegment(i)
inputIndices = self._getInputIndexesFromSynapses(synapses)
perm = self._getPermanencesFromColumn(i)
perm += permChanges[inputIndices]
Copy link
Member

Choose a reason for hiding this comment

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

some trouble in adaptSynapses() and the failing test https://travis-ci.org/numenta/nupic/jobs/59624260#L3493

Copy link
Contributor Author

Choose a reason for hiding this comment

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

some trouble in adaptSynapses() and the failing test https://travis-ci.org/numenta/nupic/jobs/59624260#L3493

Yep, I did fix it.

@david-ragazzi david-ragazzi force-pushed the use_connections_in_SP branch 2 times, most recently from 331693f to 11113e1 Compare April 26, 2015 01:31
@chetan51
Copy link
Contributor

@david-ragazzi You can now consolidate this PR with #2080.

@david-ragazzi david-ragazzi force-pushed the use_connections_in_SP branch 2 times, most recently from aab0b2b to 031c122 Compare April 27, 2015 22:39
@david-ragazzi
Copy link
Contributor Author

@david-ragazzi You can now consolidate this PR with #2080.

Done.

@cogmission
Copy link
Contributor

@david-ragazzi @rhyolight Was this merged along with #2080 ?

@david-ragazzi
Copy link
Contributor Author

@david-ragazzi @rhyolight Was this merged along with #2080 ?

Nope. #2080 was only to move Connections from TemporalMemory (which was a partial change of this PR, but I removed)... This PR is about re-use connections in SP..

@cogmission Keep on eye on this, because it's important to htm.java (this will simply it).

This change also is important to nupic.studio, because after it is merged, NS will get status of a HTM using only connections instance (as already happens with TemporalMemory, and no more handling the permanences sparse matrix). Probably this PR need include a getConnections() method to avoid make users use directly sp._connections.

@david-ragazzi david-ragazzi force-pushed the use_connections_in_SP branch from 031c122 to 7bd824c Compare April 28, 2015 23:09
david-ragazzi
david-ragazzi reviewed May 6, 2015
View reviewed changes
nupic/research/spatial_pooler.py
@@ -1308,7 +1358,9 @@ def _calculateOverlap(self, inputVector):
the spatial pooler.
"""
overlaps = numpy.zeros(self._numColumns).astype(realDType)
self._connectedSynapses.rightVecSumAtNZ_fast(inputVector, overlaps)
for i in xrange(self._numColumns):
inputIndices = self._getInputIndicesFromSynapsesForColumn(i)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: I need pass onlyConnected=True here.

@cogmission
Copy link
Contributor

Does anyone know the status of this?

@breznak
Copy link
Member

breznak commented May 20, 2015 via email

@cogmission
Copy link
Contributor

Ah thanks!

@chetan51
Copy link
Contributor

Serialization for Connections is completed: #2100

@david-ragazzi
Copy link
Contributor Author

Serialization for Connections is completed: #2100

I think this is not enough because models saved with pickle need be re-serialized to capnproto using the script addressed by #1418.

@david-ragazzi david-ragazzi changed the title Re-use Connections class in SP Fixes #1903 - Re-use Connections class in SP Jun 8, 2015
david-ragazzi added 2 commits July 7, 2015 02:49
@chetan51 chetan51 mentioned this pull request Jul 9, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@david-ragazzi david-ragazzi

Labels
status:blocked subject:algorithms type:enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Re-use Connections class in SpatialPooler class
4 participants
@david-ragazzi @breznak @chetan51 @cogmission