From 00da494cc4852d78c68d5f91d19887109bbfd0c6 Mon Sep 17 00:00:00 2001 From: Marek Otahal Date: Mon, 25 Feb 2019 10:30:47 +0100 Subject: [PATCH 1/4] SP: better checks for setters and constructor params - uses same codepath for constructor and set* methods - more strict params checks --- src/nupic/algorithms/SpatialPooler.cpp | 66 ++++++++++++++++---------- src/nupic/algorithms/SpatialPooler.hpp | 20 +++++--- 2 files changed, 53 insertions(+), 33 deletions(-) diff --git a/src/nupic/algorithms/SpatialPooler.cpp b/src/nupic/algorithms/SpatialPooler.cpp index 6abf03c2bb..9febb22847 100644 --- a/src/nupic/algorithms/SpatialPooler.cpp +++ b/src/nupic/algorithms/SpatialPooler.cpp @@ -89,7 +89,8 @@ SpatialPooler::SpatialPooler() { } SpatialPooler::SpatialPooler( - const vector inputDimensions, const vector columnDimensions, + const vector inputDimensions, + const vector columnDimensions, UInt potentialRadius, Real potentialPct, bool globalInhibition, Real localAreaDensity, Int numActiveColumnsPerInhArea, UInt stimulusThreshold, Real synPermInactiveDec, Real synPermActiveInc, @@ -136,6 +137,8 @@ UInt SpatialPooler::getPotentialRadius() const { return potentialRadius_; } void SpatialPooler::setPotentialRadius(UInt potentialRadius) { NTA_CHECK(potentialRadius < numInputs_); potentialRadius_ = potentialRadius; + NTA_CHECK((UInt)(potentialPct_ * potentialRadius_) >= 1u) << "SP: at least 1 input synapse must be able to activate from potential pool."; + NTA_CHECK(stimulusThreshold_ <= potentialPct_ * potentialRadius) << "Stimulus threshold must be <= than the number of possibly active input synapses per segment."; } Real SpatialPooler::getPotentialPct() const { return potentialPct_; } @@ -143,6 +146,8 @@ Real SpatialPooler::getPotentialPct() const { return potentialPct_; } void SpatialPooler::setPotentialPct(Real potentialPct) { NTA_CHECK(potentialPct > 0.0f && potentialPct <= 1.0f); potentialPct_ = potentialPct; + NTA_CHECK((UInt)(potentialPct_ * potentialRadius_) >= 1u) << "SP: at least 1 input synapse must be able to activate from potential pool."; + NTA_CHECK(stimulusThreshold_ <= potentialPct_ * potentialRadius_) << "Stimulus threshold must be <= than the number of possibly active input synapses per segment."; } bool SpatialPooler::getGlobalInhibition() const { return globalInhibition_; } @@ -156,7 +161,7 @@ Int SpatialPooler::getNumActiveColumnsPerInhArea() const { } void SpatialPooler::setNumActiveColumnsPerInhArea(UInt numActiveColumnsPerInhArea) { - NTA_CHECK(numActiveColumnsPerInhArea > 0u && numActiveColumnsPerInhArea <= numColumns_); //TODO this boundary could be smarter + NTA_CHECK(numActiveColumnsPerInhArea > 0u && numActiveColumnsPerInhArea <= numColumns_); numActiveColumnsPerInhArea_ = numActiveColumnsPerInhArea; localAreaDensity_ = DISABLED; //MUTEX with localAreaDensity } @@ -172,6 +177,7 @@ void SpatialPooler::setLocalAreaDensity(Real localAreaDensity) { UInt SpatialPooler::getStimulusThreshold() const { return stimulusThreshold_; } void SpatialPooler::setStimulusThreshold(UInt stimulusThreshold) { + NTA_CHECK(stimulusThreshold_ <= potentialPct_ * potentialRadius_) << "Stimulus threshold must be <= than the number of possibly active input synapses per segment."; stimulusThreshold_ = stimulusThreshold; } @@ -227,6 +233,8 @@ Real SpatialPooler::getSynPermActiveInc() const { return synPermActiveInc_; } void SpatialPooler::setSynPermActiveInc(Real synPermActiveInc) { NTA_CHECK( synPermActiveInc > connections::minPermanence ); NTA_CHECK( synPermActiveInc <= connections::maxPermanence ); + if(synPermActiveInc >= synPermConnected_) + NTA_WARN << "SP: synPermActiveInc will learn in just one occurance."; synPermActiveInc_ = synPermActiveInc; } @@ -237,6 +245,8 @@ Real SpatialPooler::getSynPermInactiveDec() const { void SpatialPooler::setSynPermInactiveDec(Real synPermInactiveDec) { NTA_CHECK( synPermInactiveDec >= connections::minPermanence ); NTA_CHECK( synPermInactiveDec <= connections::maxPermanence ); + if(synPermInactiveDec >= connections::maxPermanence - synPermConnected_) + NTA_WARN << "SP: synPermInactiveDec will unlearn in just one example"; synPermInactiveDec_ = synPermInactiveDec; } @@ -247,6 +257,7 @@ Real SpatialPooler::getSynPermBelowStimulusInc() const { void SpatialPooler::setSynPermBelowStimulusInc(Real synPermBelowStimulusInc) { NTA_CHECK( synPermBelowStimulusInc > connections::minPermanence ); NTA_CHECK( synPermBelowStimulusInc <= connections::maxPermanence ); + NTA_CHECK( synPermBelowStimulusInc <= synPermActiveInc_ * 0.5 ) << "Inactive growth should be << than for active synapses."; synPermBelowStimulusInc_ = synPermBelowStimulusInc; } @@ -415,32 +426,32 @@ void SpatialPooler::initialize( } NTA_CHECK(numColumns_ > 0); NTA_CHECK(numInputs_ > 0); - NTA_CHECK(inputDimensions_.size() == columnDimensions_.size()); + NTA_CHECK(inputDimensions_.size() == columnDimensions_.size()) << "Input and SpatialPooler must have same dimensionality."; - NTA_CHECK((numActiveColumnsPerInhArea > 0 && localAreaDensity < 0) || - (localAreaDensity > 0 && localAreaDensity <= MAX_LOCALAREADENSITY - && numActiveColumnsPerInhArea < 0) - ) << numActiveColumnsPerInhArea << " vs " << localAreaDensity; - numActiveColumnsPerInhArea_ = numActiveColumnsPerInhArea; - localAreaDensity_ = localAreaDensity; + NTA_CHECK((numActiveColumnsPerInhArea > 0 || localAreaDensity > 0.0f) && !(localAreaDensity > 0.0f && numActiveColumnsPerInhArea > 0)) << "exactly one of these must be enabled"; + if(numActiveColumnsPerInhArea > 0) { + setNumActiveColumnsPerInhArea(numActiveColumnsPerInhArea); + } else { + setLocalAreaDensity(localAreaDensity); + } rng_ = Random(seed); - potentialRadius_ = potentialRadius > numInputs_ ? numInputs_ : potentialRadius; - NTA_CHECK(potentialPct > 0 && potentialPct <= 1); - potentialPct_ = potentialPct; + setPotentialRadius(potentialRadius); + setPotentialPct(potentialPct); globalInhibition_ = globalInhibition; - stimulusThreshold_ = stimulusThreshold; - synPermInactiveDec_ = synPermInactiveDec; - synPermActiveInc_ = synPermActiveInc; - synPermBelowStimulusInc_ = synPermConnected / 10.0f; + setStimulusThreshold(stimulusThreshold); + NTA_CHECK(0.0 < synPermConnected && synPermConnected < 1.0); synPermConnected_ = synPermConnected; - minPctOverlapDutyCycles_ = minPctOverlapDutyCycles; - dutyCyclePeriod_ = dutyCyclePeriod; - boostStrength_ = boostStrength; - spVerbosity_ = spVerbosity; - wrapAround_ = wrapAround; - updatePeriod_ = 50u; + setSynPermInactiveDec(synPermInactiveDec); + setSynPermActiveInc(synPermActiveInc); + setSynPermBelowStimulusInc(synPermConnected / 10.0f); + setMinPctOverlapDutyCycles(minPctOverlapDutyCycles); + setDutyCyclePeriod(dutyCyclePeriod); + setBoostStrength(boostStrength); + setSpVerbosity(spVerbosity); + setWrapAround(wrapAround); + setUpdatePeriod(50u); initConnectedPct_ = 0.5f; iterationNum_ = 0u; iterationLearnNum_ = 0u; @@ -584,8 +595,7 @@ vector SpatialPooler::initMapPotential_(UInt column, bool wrapAround) { columnInputs.push_back(input); } } else { - for (UInt input : - Neighborhood(centerInput, potentialRadius_, inputDimensions_)) { + for (UInt input : Neighborhood(centerInput, potentialRadius_, inputDimensions_)) { columnInputs.push_back(input); } } @@ -601,13 +611,17 @@ Real SpatialPooler::initPermConnected_() { Real p = synPermConnected_ + (Real)((connections::maxPermanence - synPermConnected_) * rng_.getReal64()); - return round5_(p); + p = round5_(p); + NTA_ASSERT(p >= synPermConnected_); + return p; } Real SpatialPooler::initPermNonConnected_() { Real p = (Real)(synPermConnected_ * rng_.getReal64()); - return round5_(p); + p = round5_(p); + NTA_ASSERT(p < synPermConnected_); + return p; } diff --git a/src/nupic/algorithms/SpatialPooler.hpp b/src/nupic/algorithms/SpatialPooler.hpp index 32d510703f..864526a1b9 100644 --- a/src/nupic/algorithms/SpatialPooler.hpp +++ b/src/nupic/algorithms/SpatialPooler.hpp @@ -41,7 +41,7 @@ namespace algorithms { namespace spatial_pooler { using namespace std; -static const int DISABLED = -1; //value denoting a feature is disabled +static const int DISABLED = 0; //value denoting a feature is disabled /** * CLA spatial pooler implementation in C++. @@ -70,14 +70,20 @@ class SpatialPooler : public Serializable { public: SpatialPooler(); - SpatialPooler(const vector inputDimensions, const vector columnDimensions, - UInt potentialRadius = 16u, Real potentialPct = 0.5f, - bool globalInhibition = true, Real localAreaDensity = DISABLED, + SpatialPooler(const vector inputDimensions, + const vector columnDimensions, + UInt potentialRadius = 16u, + Real potentialPct = 0.5f, + bool globalInhibition = true, + Real localAreaDensity = DISABLED, Int numActiveColumnsPerInhArea = 10u, - UInt stimulusThreshold = 0u, Real synPermInactiveDec = 0.008f, - Real synPermActiveInc = 0.05f, Real synPermConnected = 0.1f, + UInt stimulusThreshold = 0u, + Real synPermInactiveDec = 0.008f, + Real synPermActiveInc = 0.05f, + Real synPermConnected = 0.1f, Real minPctOverlapDutyCycles = 0.001f, - UInt dutyCyclePeriod = 1000u, Real boostStrength = 0.0f, + UInt dutyCyclePeriod = 1000u, + Real boostStrength = 0.0f, Int seed = 1, UInt spVerbosity = 0u, bool wrapAround = true); virtual ~SpatialPooler() {} From 7405fd80e18f0cc4eba8295166abb09bbe92d239 Mon Sep 17 00:00:00 2001 From: Marek Otahal Date: Thu, 11 Jan 2018 04:17:10 +0100 Subject: [PATCH 2/4] SP: add test for different constructor vs setter behavior SP behaves differently when constructor vs setters are used to set the same parameters! --- .../unit/algorithms/SpatialPoolerTest.cpp | 54 ++++++++++++++++++- 1 file changed, 52 insertions(+), 2 deletions(-) diff --git a/src/test/unit/algorithms/SpatialPoolerTest.cpp b/src/test/unit/algorithms/SpatialPoolerTest.cpp index 9fed69fa58..d3873cdb17 100644 --- a/src/test/unit/algorithms/SpatialPoolerTest.cpp +++ b/src/test/unit/algorithms/SpatialPoolerTest.cpp @@ -2005,14 +2005,64 @@ TEST(SpatialPoolerTest, testSerialization2) { } } } - cout << "Timing for SpatialPooler serialization (smaller is better):" << endl; cout << "Stream: " << testTimer.getElapsed() << endl; - remove("outC.stream"); } +TEST(SpatialPoolerTest, testDifferentConstructorVsSetterBehavior) +{ + /** this test exposes wrong behavior, where SP created via constructor + behaves differently to a SP via setters (setXXX()), both with the same + params. + */ + SpatialPooler spConstruct{std::vector{10} /* input*/, std::vector{2048}/* SP output cols XXX sensitive*/, + /*pot radius*/ 20, //each col sees + /*pot pct*/ 0.5, //XXX sensitive + /*global inhibition*/ false, //XXX sensitive + /*Real localAreaDensity=*/0.02, //2% active cols + /*UInt numActiveColumnsPerInhArea=*/0, //mutex with above ^^ //XXX sensitive +}; + + SpatialPooler sp{std::vector{10} /* input*/, std::vector{2048}/* SP output cols */}; +sp.setPotentialRadius(20); +sp.setPotentialPct(0.5); +sp.setGlobalInhibition(false); +sp.setLocalAreaDensity(0.02); //2% active cols +sp.setNumActiveColumnsPerInhArea(0); //mutex with above ^^ + + +// EXPECT_EQ(spConstruct, sp); //FIXME how compare 2 SP +check_spatial_eq(spConstruct, sp); +} + + + +TEST(SpatialPoolerTest, testSaveLoad) { + const char* filename = "SpatialPoolerSerialization.tmp"; + SpatialPooler sp1, sp2; + UInt numInputs = 6; + UInt numColumns = 12; + setup(sp1, numInputs, numColumns); + + ofstream outfile; + outfile.open(filename); + sp1.save(outfile); + outfile.close(); + + ifstream infile (filename); + sp2.load(infile); + infile.close(); + + ASSERT_NO_FATAL_FAILURE( + check_spatial_eq(sp1, sp2)); + + int ret = ::remove(filename); + ASSERT_TRUE(ret == 0) << "Failed to delete " << filename; +} + + TEST(SpatialPoolerTest, testConstructorVsInitialize) { // Initialize SP using the constructor SpatialPooler sp1( From fb2f89c38b685e9045d60e2bdf8cbd99c8edd4f4 Mon Sep 17 00:00:00 2001 From: Marek Otahal Date: Mon, 25 Feb 2019 12:41:58 +0100 Subject: [PATCH 3/4] merge cleanup --- .../unit/algorithms/SpatialPoolerTest.cpp | 41 ++++--------------- 1 file changed, 8 insertions(+), 33 deletions(-) diff --git a/src/test/unit/algorithms/SpatialPoolerTest.cpp b/src/test/unit/algorithms/SpatialPoolerTest.cpp index d3873cdb17..7b9e1980ca 100644 --- a/src/test/unit/algorithms/SpatialPoolerTest.cpp +++ b/src/test/unit/algorithms/SpatialPoolerTest.cpp @@ -2023,43 +2023,18 @@ TEST(SpatialPoolerTest, testDifferentConstructorVsSetterBehavior) /*global inhibition*/ false, //XXX sensitive /*Real localAreaDensity=*/0.02, //2% active cols /*UInt numActiveColumnsPerInhArea=*/0, //mutex with above ^^ //XXX sensitive -}; + }; SpatialPooler sp{std::vector{10} /* input*/, std::vector{2048}/* SP output cols */}; -sp.setPotentialRadius(20); -sp.setPotentialPct(0.5); -sp.setGlobalInhibition(false); -sp.setLocalAreaDensity(0.02); //2% active cols -sp.setNumActiveColumnsPerInhArea(0); //mutex with above ^^ + sp.setPotentialRadius(20); + sp.setPotentialPct(0.5); + sp.setGlobalInhibition(false); + sp.setLocalAreaDensity(0.02); //2% active cols + sp.setNumActiveColumnsPerInhArea(0); //mutex with above ^^ -// EXPECT_EQ(spConstruct, sp); //FIXME how compare 2 SP -check_spatial_eq(spConstruct, sp); -} - - - -TEST(SpatialPoolerTest, testSaveLoad) { - const char* filename = "SpatialPoolerSerialization.tmp"; - SpatialPooler sp1, sp2; - UInt numInputs = 6; - UInt numColumns = 12; - setup(sp1, numInputs, numColumns); - - ofstream outfile; - outfile.open(filename); - sp1.save(outfile); - outfile.close(); - - ifstream infile (filename); - sp2.load(infile); - infile.close(); - - ASSERT_NO_FATAL_FAILURE( - check_spatial_eq(sp1, sp2)); - - int ret = ::remove(filename); - ASSERT_TRUE(ret == 0) << "Failed to delete " << filename; + // EXPECT_EQ(spConstruct, sp); //FIXME how compare 2 SP + check_spatial_eq(spConstruct, sp); } From 5849e1ea5b0d12402fb1b78191b319660250bf56 Mon Sep 17 00:00:00 2001 From: Marek Otahal Date: Mon, 25 Feb 2019 12:49:30 +0100 Subject: [PATCH 4/4] SP: warn about potential radius out of dimmensions --- src/nupic/algorithms/SpatialPooler.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/nupic/algorithms/SpatialPooler.cpp b/src/nupic/algorithms/SpatialPooler.cpp index 9febb22847..742f1ece04 100644 --- a/src/nupic/algorithms/SpatialPooler.cpp +++ b/src/nupic/algorithms/SpatialPooler.cpp @@ -135,7 +135,12 @@ UInt SpatialPooler::getNumInputs() const { return numInputs_; } UInt SpatialPooler::getPotentialRadius() const { return potentialRadius_; } void SpatialPooler::setPotentialRadius(UInt potentialRadius) { - NTA_CHECK(potentialRadius < numInputs_); + NTA_CHECK(potentialRadius < numInputs_/2); + for(const auto dim: inputDimensions_) { + if(potentialRadius > (dim/2) +1) { + NTA_WARN << "potentialRadius >= one of the dimensions: " << dim; + } + } potentialRadius_ = potentialRadius; NTA_CHECK((UInt)(potentialPct_ * potentialRadius_) >= 1u) << "SP: at least 1 input synapse must be able to activate from potential pool."; NTA_CHECK(stimulusThreshold_ <= potentialPct_ * potentialRadius) << "Stimulus threshold must be <= than the number of possibly active input synapses per segment.";