Connections create synapse improvements #746

breznak · 2019-11-06T12:10:46Z

fixes the correctness of creating new synapse on segment
moved to Connections, simplified code

as it was never used, use existing Connections.cellForSegment() instead

The logic was moved from TM.growSynapses_() to Connections.createSynapse() as it's generally applicable and correct. We want to avoid creating synapses to already existing presynaptic cells (on the same segment) as that breaks the binary nature of synapses. Advantage of this is simplified implementation and much cleaner code in TM.

breznak

Please review below,

src/examples/hotgym/HelloSPTP.cpp

breznak · 2019-11-06T12:42:45Z

src/htm/algorithms/Connections.cpp

  //proceed to create a new segment
-  Segment segment;
-  if (!destroyedSegments_.empty() ) { //reuse old, destroyed segs
-    segment = destroyedSegments_.back();


replacing the destroyed_ buffers. Makes code less complicated and the performance is the same.

So this will never reuse a destroyed segment?

IMO: in a typical application, segments should never be destroyed, just created. If the HTM reaches full capacity and needs to make room for new information, then the HTM should remove synapses from a segment instead of removing the segment itself.

So this will never reuse a destroyed segment?
IMO: in a typical application, segments should never be destroyed, just created.

yes. if you want, I can move this commit to a separete PR to discuss there? You're right we almost never remove segments/synapses (cca 0.X% on hotgym benchmark).

If the HTM reaches full capacity and needs to make room for new information, then the HTM should remove synapses from a segment instead of removing the segment itself.

if that is valid requirement, we should remove altogether the method destroySegment().

HTM reaches full capacity and needs to make room

when is this? When synapses on a segment are full, when synapses on all segments are maxed, ...?

should remove synapses from a segment instead of removing the segment itself.

then we'd need to limit this pruning to some reasonable value (keep more synapses than activation threshold), otherwise the seg can never be active again.

currently we do this the other way, that such segment dies off (which I think is more plausible than setting a limit on a number of syns that need be left).

breznak · 2019-11-06T12:44:36Z

src/htm/algorithms/Connections.cpp

+  for (const Synapse& syn : synapsesForSegment(segment)) {
+    const CellIdx existingPresynapticCell = dataForSynapse(syn).presynapticCell; //TODO 1; add way to get all presynaptic cells for segment (fast)
+    if (presynapticCell == existingPresynapticCell) {
+      return syn; //synapse (connecting to this presyn cell) already exists on the segment; don't create a new one, exit early and return the existing


1/ moving code for avoiding duplicate presynaptic connections on the same segment from TM,

implementation is easier (&faster about 8% for TM)

2/ was a bug that it was checked only in TM, is applicable generally and should be part of Connections.

breznak · 2019-11-06T12:46:34Z

src/htm/algorithms/Connections.hpp

-   * @param cells
-   * Output array with the same length as 'segments'
-   */
-  void mapSegmentsToCells(const Segment *segments_begin,


removed unused method, cellForSegment() provides the same.

breznak · 2019-11-06T12:47:13Z

src/htm/algorithms/TemporalMemory.cpp

  Int overrun = static_cast<Int>(connections.numSynapses(segment) + nActual - maxSynapsesPerSegment_);
  if (overrun > 0) {
-    connections.destroyMinPermanenceSynapses(segment, static_cast<Int>(overrun), prevWinnerCells);
+    connections.destroyMinPermanenceSynapses(segment, static_cast<Int>(overrun), prevWinnerCells); //TODO move this functionality to Connections.createSynapse() 


OT: for next PR

src/htm/algorithms/TemporalMemory.cpp

breznak · 2019-11-06T12:55:56Z

src/test/unit/algorithms/ConnectionsTest.cpp

+  connections.createSynapse(segment, 51, 0.34f);
+  const size_t numSynapses = connections.synapsesForSegment(segment).size(); //created 2 synapses above
+  connections.createSynapse(segment, 50, 0.48f); //attempt to create already existing synapse (to presyn cell "50") -> skips as no duplication should happen
+  ASSERT_EQ(connections.synapsesForSegment(segment).size(), numSynapses) << "Duplicit synapses should not be created!";


test the new "check dupes, skip" behavior of createSynapse.

ctrl-z-9000-times

I agree that this is a better way to organize the code.

Thank you

src/htm/algorithms/TemporalMemory.cpp

src/examples/hotgym/HelloSPTP.cpp

ctrl-z-9000-times · 2019-11-06T13:34:19Z

src/htm/algorithms/Connections.cpp

  //proceed to create a new segment
-  Segment segment;
-  if (!destroyedSegments_.empty() ) { //reuse old, destroyed segs
-    segment = destroyedSegments_.back();


So this will never reuse a destroyed segment?

IMO: in a typical application, segments should never be destroyed, just created. If the HTM reaches full capacity and needs to make room for new information, then the HTM should remove synapses from a segment instead of removing the segment itself.

breznak · 2019-11-11T11:16:34Z

Please re-review when you have time, I have some follow up work on Connections, and I'd like to keep this PR small.

dkeeney

I find no problem with this.
@ctrl-z-9000-times should be the one to take a look at this but I will go ahead and approve in case it is holding you up.

ctrl-z-9000-times

I don't have any problems with this, although I have not proof read the code. You should merge this with Mr Keeney's approval.

breznak · 2019-11-12T15:30:24Z

Thank you both for reviewing, my quite confident this code will be fine 👍
Glad to merge this, it was holding back my research on connections.

breznak added 3 commits November 5, 2019 08:28

Connections.mapSegmentsToCells removed

8b371dc

as it was never used, use existing Connections.cellForSegment() instead

Connections: remove destroyedSynapse/Segments_ buffers

06ca0c2

breznak added enhancement New feature or request in_progress optimization labels Nov 6, 2019

breznak self-assigned this Nov 6, 2019

breznak commented Nov 6, 2019

View reviewed changes

breznak requested review from ctrl-z-9000-times and dkeeney November 6, 2019 12:56

breznak added ready bug Something isn't working and removed in_progress labels Nov 6, 2019

ctrl-z-9000-times reviewed Nov 6, 2019

View reviewed changes

breznak requested a review from ctrl-z-9000-times November 8, 2019 12:31

dkeeney approved these changes Nov 12, 2019

View reviewed changes

Merge branch 'master' into connections_createSynapse

183f30f

ctrl-z-9000-times approved these changes Nov 12, 2019

View reviewed changes

breznak merged commit 686abdf into master Nov 12, 2019

breznak deleted the connections_createSynapse branch November 12, 2019 15:30

breznak mentioned this pull request Nov 12, 2019

Connections: replace destroyMinPermSynapses() with plausible decay #751

Open

3 tasks

Connections create synapse improvements #746

Connections create synapse improvements #746

Uh oh!

Conversation

breznak commented Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

breznak left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

breznak Nov 6, 2019

Choose a reason for hiding this comment

Uh oh!

ctrl-z-9000-times Nov 6, 2019

Choose a reason for hiding this comment

Uh oh!

breznak Nov 7, 2019

Choose a reason for hiding this comment

Uh oh!

breznak Nov 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

breznak Nov 6, 2019

Choose a reason for hiding this comment

Uh oh!

breznak Nov 6, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

breznak Nov 6, 2019

Choose a reason for hiding this comment

Uh oh!

ctrl-z-9000-times left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ctrl-z-9000-times Nov 6, 2019

Choose a reason for hiding this comment

Uh oh!

breznak commented Nov 11, 2019

Uh oh!

dkeeney left a comment

Choose a reason for hiding this comment

Uh oh!

ctrl-z-9000-times left a comment

Choose a reason for hiding this comment

Uh oh!

breznak commented Nov 12, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

breznak commented Nov 6, 2019 •

edited

Loading

breznak Nov 6, 2019 •

edited

Loading