Fix Connections segment pruning #601

breznak · 2019-08-02T14:26:54Z

fixes segment pruning in Connections
- properly used in SP, TM
SP with enabled synapse pruning is not platform-independent!
bugfixes for dataForSynapse(), dataForSegment() + tests
fix Connections de/serialization
fix TM maxSegmentsPerCell check

new segmentThreshold arg introduced, as connectedThreshold_ has different, incompatible meaning.

in adaptSegment

moved from being static

to private method

instead of lower_bound, caused err in Debug. Seems faster

breznak

Please review when you find time.
Question to consider: make an equivalent of stimulusThreshold_ in Connections? (currently passed as arg to the methods)

src/htm/algorithms/Connections.cpp

breznak · 2019-08-02T14:28:51Z

src/htm/algorithms/Connections.cpp

@@ -433,7 +429,8 @@ void Connections::adaptSegment(const Segment segment,
                               const SDR &inputs,
                               const Permanence increment,
                               const Permanence decrement, 
-			       const bool pruneZeroSynapses)
+			       const bool pruneZeroSynapses, 
+			       const UInt segmentThreshold)


these 2 options could be merged together, but I like to keep them separate: first gates synapse pruning, the latter segment pruning.

breznak · 2019-08-02T14:29:31Z

src/htm/algorithms/Connections.cpp

+  if(!pruneZeroSynapses) {
+    NTA_ASSERT(segmentThreshold == 0) << "Setting segmentThreshold only makes sense when pruneZeroSynapses is allowed.";
+  }
+  if(pruneZeroSynapses and synapses.size() < segmentThreshold) { 


this was the bug I was about, connectedThreshold_ was used incorrectly.

src/htm/algorithms/SpatialPooler.cpp

src/htm/algorithms/TemporalMemory.hpp

src/examples/hotgym/HelloSPTP.cpp

breznak · 2019-08-02T20:59:20Z

a change I introduced, probably the find with relaxed ordering, results in undeterministic results. Needs fixing.

and only keep count of segments that were destroyed. Pros: - simpler code - fixed deterministm for (serialized connections & using pruning) - maybe better cache utilization, as vector<SegmentData> is continuous now Cons: - larger memory utilization, as segments_ and segmentData_ vectors now have holes in it

lower_bound failed on "does not parition", find() is fine, and simpler code.

as a premature optimization, keep only as counter of num destroyed. - cleaner code - faster - but slightly wastes memory (but we only destroy 0.3% synapses on MNIST)

as pruning seems not platform-independent.

the debug code was called after! the segment has been already removed

by setting segmentThreshold param. This should be ON, but there are some issues with platform independent reproducible results

for platform-independent build?

add arguments with defaults

after synapse is destroyed by destroySynapse(syn), its data were still accessible! Which could lead to false operations. Add check to dataForSynapse() that fails if synapse has been removed. Detail: synapseExists_() is available, improve its performance to be widely usable (by dataForSynapse()). Tests for create/destroySynapse

if the new permanence is higher that the permanence of the old (same) synapse, update to the higher value.

that only test the functionality of "main" Connections, which are already tested. Done due to changes in the main tests, which would have to be rewritten here as well. The "advanced" tests should cover all the special functionality not part of the main.

breznak

@dkeeney please have a look (even if only at the high-level changes)?
This is an old PR that accumulated many fixes that were quite hard to debug, all tests pass so I assume it's safe to merge.

breznak · 2020-06-03T21:01:23Z

bindings/py/tests/algorithms/connections_test.py

@@ -163,6 +165,66 @@ def testAdaptShouldDecrementSynapses(self):
        self.assertEqual(presynamptic_cells, presynaptic_input_set, "Missing synapses")


+
+  def testCreateSynapse(self):


added new tests

breznak · 2020-06-03T21:03:43Z

py/tests/advanced/algorithms/advanced_connections_test.py

@@ -38,207 +38,6 @@ def _getPresynapticCells(self, connections, segment, threshold):
        return set([connections.presynapticCellForSynapse(synapse) for synapse in connections.synapsesForSegment(segment) 
                    if connections.permanenceForSynapse(synapse) >= threshold])

-    def testAdaptShouldNotRemoveSegments(self):
-        """
-        Test that connections are generated on predefined segments.


FYI @fcr I'm removing the "duplicit" tests for advanced_connections.py for methods that are not changed by this Connection implementation, and are already tested in the "main" tests. As I made changes to the tests and would have to port them here.

breznak · 2020-06-03T21:07:56Z

src/htm/algorithms/Connections.cpp

+      //1. just keep the older (former default)
+      //2. throw an error (ideally, user should not createSynapse() but rather updateSynapsePermanence())
+      //3. create a duplicit new synapse -- NO. This is the only choice that is incorrect! HTM works on binary synapses, duplicates would break that.
+      //4. update to the max of the permanences (default)


documented and changed the default strategy for createSynapse() if such synapse already exists. All four alternatives discussed above,

former default: leave as is;

now: update permanence if the new perm would be larger.

...impact is not really seen in our unit-tests as none trigger this "duplicit synapse created" branch.

breznak · 2020-06-03T21:10:36Z

src/htm/algorithms/Connections.cpp

+
+  } else {
+  //quick method. Relies on hack in destroySynapse() where we set synapseData.permanence == -1
+  return synapses_[synapse].permanence != -1;


this code above is quite slow, but is needed and needed in a heavily used dataForSynapse(), therefore I created this "fast" variant/hack.

breznak · 2020-06-03T21:11:19Z

src/htm/algorithms/Connections.cpp

+		                          segmentData.synapses.cend(),
+					  synapse,
+					  [&](const Synapse a, const Synapse b) -> bool { return dataForSynapse(a).id < dataForSynapse(b).id;}
+					  ); 


just whitespace formatting

breznak · 2020-06-03T21:13:42Z

src/htm/algorithms/Connections.hpp

@@ -374,7 +378,8 @@ class Connections : public Serializable
   *
   * @retval Synapse data.
   */
-  const SynapseData &dataForSynapse(const Synapse synapse) const {
+  inline const SynapseData& dataForSynapse(const Synapse synapse) const {
+    NTA_CHECK(synapseExists_(synapse, true));


needed check for dataForSynapse/Segment(). Otherwise, the data could be accessed even after removing the synapse/segment with destroy*().

breznak · 2020-06-03T21:14:33Z

src/htm/algorithms/Connections.hpp

+    //the following member must not be serialized (so is set to =0).
+    //That is because of we serialize only active segments & synapses,
+    //excluding the "destroyed", so those fields start empty.
+//!    ar(CEREAL_NVP(destroyedSegments_));


fixed bug in serialization of connections.

breznak · 2020-06-03T21:16:23Z

src/htm/algorithms/TemporalMemory.cpp

@@ -206,7 +206,7 @@ void TemporalMemory::activatePredictedColumn_(
    do {
      if (learn) { 
        connections_.adaptSegment(*activeSegment, prevActiveCells,
-                     permanenceIncrement_, permanenceDecrement_, true);
+                     permanenceIncrement_, permanenceDecrement_, true, minThreshold_);


FIX: TM now honors the "max segments per cell" logic.

dkeeney

I found no problems while doing a high level review.
I have not been following the changes that you are making to the Connections algorithm but I assume they are all good.

breznak · 2020-06-04T06:08:17Z

Thank you, David!

I have not been following the changes that you are making to the Connections algorithm but I assume they are all good.

i've just double checked and the changes to Conn are all fixes, so we're good.

breznak added 7 commits August 2, 2019 12:44

Connections::adaptSegment fix segment pruning

c672f49

new segmentThreshold arg introduced, as connectedThreshold_ has different, incompatible meaning.

SP & TM use synapse & segment pruning from Connections

3f4999c

in adaptSegment

TM.punishPredictedColumn_() is a private method of TM

1c3a684

moved from being static

TM:activatePredictedColumns() moved from static

c47b7a4

to private method

TM::burstColumn() made private from static

9ce19f9

TM:growSynapses() made private from static

d3ba477

Connections:destroySynapse fixup, use find

4be092b

instead of lower_bound, caused err in Debug. Seems faster

breznak added ready fix labels Aug 2, 2019

breznak requested review from dkeeney and ctrl-z-9000-times August 2, 2019 14:26

breznak self-assigned this Aug 2, 2019

breznak commented Aug 2, 2019

View reviewed changes

fix deterministic test results

6f535c4

breznak commented Aug 2, 2019

View reviewed changes

src/examples/hotgym/HelloSPTP.cpp Outdated Show resolved Hide resolved

reintroduce lower_bound w deterministic ordering

b6d04bb

breznak mentioned this pull request Aug 3, 2019

TM static methods made private #604

Merged

1 task

breznak removed the ready label Aug 9, 2019

breznak added 9 commits August 9, 2019 17:26

Merge branch 'master_community' into fix_conn_stimulusThreshold

b7fa36e

Merge branch 'master_community' into fix_conn_stimulusThreshold

4a571bb

cleanup createSegment

36608e5

assert

f35daf7

Connections: fix crash in destroySynapse

24b39be

lower_bound failed on "does not parition", find() is fine, and simpler code.

Connections: removed vector destroyedSynapses_

0b53cf7

as a premature optimization, keep only as counter of num destroyed. - cleaner code - faster - but slightly wastes memory (but we only destroy 0.3% synapses on MNIST)

Merge branch 'master_community' into fix_conn_stimulusThreshold

f2e16ef

fix exact output results from CI

2d1bd5a

breznak added the in_progress label Aug 10, 2019

SP cannot call adaptSegment with pruning,

a400325

as pruning seems not platform-independent.

Merge branch 'conn_createSyn2' into fix_conn_stimulusThreshold

03e2fc5

breznak added the bug Something isn't working label Jun 3, 2020

breznak added 2 commits June 3, 2020 13:10

fix bug in Conn.pruneLRUSegment_()

b91144d

the debug code was called after! the segment has been already removed

SP: enable segment pruning for Conn.adaptSegment

46e4ca5

by setting segmentThreshold param. This should be ON, but there are some issues with platform independent reproducible results

breznak force-pushed the fix_conn_stimulusThreshold branch from 3e4ae13 to 46e4ca5 Compare June 3, 2020 11:55

breznak added 14 commits June 3, 2020 14:23

COnn revert to lower_bound with explicit ordering

b58258c

for platform-independent build?

Connections: add test for destroySynapse

6bc3dc7

COnnections: add test for createSynapse

bcefa8c

Connections adaptSegment: improve py binding

56b6a1f

add arguments with defaults

Connections: test for createSynapse

7463e5f

COnnections.createSynapse : update exiting if higher permanence

1d9518b

if the new permanence is higher that the permanence of the old (same) synapse, update to the higher value.

revert changes to SP and deterministic results

b0d1338

bump test values for new changes

1de0631

COnnections: add couple of noexcept

038a292

Connections tests for createSegment()

540f3c4

COnnections: test for destroySegment()

bf7ee11

Connections dataForSegment throws if segment removed

ffb8ee8

breznak commented Jun 3, 2020

View reviewed changes

breznak added ready and removed in_progress labels Jun 3, 2020

Merge branch 'master_community' into fix_conn_stimulusThreshold

690047c

This was referenced Jun 3, 2020

TM, SP: fix possible underflow in seed. Default random (0) seed. #799

Open

SP local inhibition optimization #783

Merged

dkeeney approved these changes Jun 3, 2020

View reviewed changes

breznak merged commit c50a669 into master Jun 4, 2020

breznak deleted the fix_conn_stimulusThreshold branch June 4, 2020 06:08

breznak mentioned this pull request Aug 5, 2020

Model constantly grows in size, implement removing synapses, segments; death #461

Open

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Connections segment pruning #601

Fix Connections segment pruning #601

breznak commented Aug 2, 2019 •

edited

Loading

breznak left a comment

breznak Aug 2, 2019

breznak Aug 2, 2019

breznak commented Aug 2, 2019

breznak left a comment

breznak Jun 3, 2020

breznak Jun 3, 2020

breznak Jun 3, 2020

breznak Jun 3, 2020

breznak Jun 3, 2020

breznak Jun 3, 2020

breznak Jun 3, 2020

breznak Jun 3, 2020

breznak Jun 3, 2020

dkeeney left a comment

breznak commented Jun 4, 2020

		@@ -163,6 +165,66 @@ def testAdaptShouldDecrementSynapses(self):
		self.assertEqual(presynamptic_cells, presynaptic_input_set, "Missing synapses")



		def testCreateSynapse(self):

Fix Connections segment pruning #601

Fix Connections segment pruning #601

Conversation

breznak commented Aug 2, 2019 • edited Loading

breznak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

breznak commented Aug 2, 2019

breznak left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dkeeney left a comment

Choose a reason for hiding this comment

breznak commented Jun 4, 2020

breznak commented Aug 2, 2019 •

edited

Loading