SP: remove numActiveColumnsPerInhArea #549

breznak · 2019-07-09T13:12:04Z

and all of its methods.
Use localAreaDensity instead.

and all of its methods. Use `localAreaDensity` instead.

breznak

Before I proceed to fixing the tests, I'd like to have your approval about this change.
I think this was an unnatural variant, compared to the density based method.

simplifies code and avoids mutex in params
our "real world" examples use the local area density method, which is kept
updated API changelog
TODO tests need fixing

breznak · 2019-07-09T13:13:54Z

src/htm/algorithms/SpatialPooler.cpp

-  Real targetDensity;
-  if (numActiveColumnsPerInhArea_ > 0) {
-    UInt inhibitionArea =
-        (UInt)(pow((Real)(2 * inhibitionRadius_ + 1), (Real)columnDimensions_.size()));


this was broken for nD SP where SP wasn't a "cube" nxnxn.

I don;t see how this is broken. The volume of an N dimensional hypercube with side length X is X ^ N.

Problem is SP's dimensions are not always a hyper-cube, rather a hyper-rectangle, eg. 1000x1x1x1 -> you get area ~ (2R)^4, compared to ~ 2R in 1D!

breznak · 2019-07-09T13:17:57Z

src/htm/algorithms/SpatialPooler.hpp

-        learn and grow their effective receptive fields, the
-        inhibitionRadius will grow, and hence the net density of the
-        active columns will *decrease*. This is in contrast to the
-        localAreaDensity method, which keeps the density of active


I think localAreaDensity should be used instead of numActiveColumnsPerInhArea, as it keeps const sparsity.

src/htm/regions/SPRegion.cpp

ctrl-z-9000-times

I agree with you assessment. Mistakes were made.

Should we remove fix this particular mistake?

Pro: density is superior to numActive
Pro: fixes the code

Con: It's work for us to do
Con: It breaks the API. Can you do some kind of git magic to tag the code base with a version number?

If your willing to do this then I will approve

re-enable numActiveColumnsPerInhArea for compatibility in NetworkAPI. But the use of that argument is discouraged! = print deprecation warning on each use - convert to localAreaDensity which is still the only one allowed internally.

breznak

Con: It breaks the API. Can you do some kind of git magic to tag the code base with a version number?

Thank you for review!
I hadn't realized this breaks the NetworkAPI which is the no-touchy API! 😊

Came with a hack for the conversion to/from numColumns to localAreaDensity in SPRegion.
That way NetworkAPI stays intact but c++ & py SPs have the-correcter (tm) API.

I'd also like blessing from @dkeeney who "keeps his eye on the API compatibility"

breznak · 2019-07-10T10:20:47Z

src/htm/regions/SPRegion.cpp

+        colsPerArea *= std::min(numColsInDim, R);
+      }
+      NTA_ASSERT(colsPerArea > 0.0f);
+      return static_cast<UInt>(round(density * colsPerArea));


I've implemented this to convert numColumns <-> localAreaDensity

breznak · 2019-07-10T10:22:35Z

src/htm/regions/SPRegion.cpp

+	dims = sp_->getColumnDimensions();
+      } else {
+        density = args_.localAreaDensity;
+	NTA_THROW << "Cannot compute from these params!";


this still fails if sp_ is not initialized. As those params I need to access (inhibitionRadius_, columnDimensions_) are not part of args_.
Otherwise this works, so I guess it's OK to keep like that considering the whole param is strongly deprecated.

breznak · 2019-07-10T10:22:57Z

src/htm/regions/SPRegion.cpp

+      Real colsPerArea = 1.0f;
+      const UInt R = (2* inhRadius +1);
+      for(const auto numColsInDim: dims) {
+        colsPerArea *= std::min(numColsInDim, R);


this is the fixed way to compute the area for nD

dkeeney · 2019-07-10T13:46:44Z

I'd also like blessing from @dkeeney who "keeps his eye on the API compatibility"

I have no problem changing or removing that parameter because the entire SPRegion object is a region that we added to Network API. It is highly unlikely that anyone will already have code that uses it and would break.

However, python users may be expecting that parameter in the original code that implemented SPRegion in .py code. I don't know if we still have that. Anyway, document in the AP_CHANGELOG that we made the change to the parameters.

breznak · 2019-07-10T13:54:46Z

I have no problem changing or removing that parameter because the entire SPRegion object is a region that we added to Network API. It is highly unlikely that anyone will already have code that uses it

oh, good point, thanks!

So what do you think of 4660174
Leave as is, or completely purge?

dkeeney · 2019-07-10T14:05:08Z

Leave as is, or completely purge?

I think we can purge it in this case.

breznak · 2019-07-10T14:13:34Z

I think we can purge it in this case.

Will do, I can just revert the last commit.

Before I go on fixing the test results, any comments on default value of localAreaDensity aka sparsity? Currently I set it to 10%, commonly we talk about 2%, and get best results with 5-10%. Not that anybody can't change it, but it'll once be default.

dkeeney · 2019-07-10T14:45:26Z

any comments on default value of localAreaDensity

I think just set it to what seems to get the best results.

This reverts commit 4660174.

localAreaDensity = 0.05 aka 5%

which should have the fastest linking speeds

when localAreaDensity, aka sparsity is set, check with column sizes that produced active output columns could be > 0

breznak

@dkeeney @ctrl-z-9000-times Please review, this should be merged quite early, before any significant TM changes.

makes hotgym test actually benchmark real TM performance (becomes 3x slower than SP)
replaces numActivePerInhArea with better sparsity, which is logical and enforces const sparsity on SP
- adds check for feasible values of sparsity with regards to size of SP (this caused many changes to tests)

breznak · 2019-07-12T12:05:19Z

CommonCompilerConfig.cmake

-                set(optimization_flags_cc ${optimization_flags_cc} -fuse-linker-plugin -flto-report -flto) #TODO fix LTO for clang
-                set(optimization_flags_lt ${optimization_flags_lt} -flto) #TODO LTO for clang too
+                set(optimization_flags_cc ${optimization_flags_cc} -fuse-linker-plugin -flto-report -flto -fno-fat-lto-objects) #TODO fix LTO for clang
+                set(optimization_flags_lt ${optimization_flags_lt} -flto -fno-fat-lto-objects) #TODO LTO for clang too


OT: added support for LLD linker, if installed. And do "thin LTO" which should make linking phase faster.

breznak · 2019-07-12T12:07:23Z

src/examples/hotgym/HelloSPTP.cpp

      };
      goldSPlocal.setSparse(deterministicSPlocal);

      SDR goldTM({COLS});
      const SDR_sparse_t deterministicTM{
-        1965 //FIXME this is a really bad representation -> improve the params
+        51, 62, 72, 77, 102, 155, 287, 306, 337, 340, 370, 493, 542, 952, 1089, 1110, 1115, 1193, 1463, 1488, 1507, 1518, 1547, 1626, 1668, 1694, 1781, 1803, 1805, 1827, 1841, 1858,1859, 1860, 1861, 1862, 1878, 1881, 1915, 1918, 1923, 1929, 1933, 1939, 1941, 1953, 1955, 1956, 1958, 1961, 1965, 1968, 1975, 1976, 1980, 1981, 1985, 1986, 1987, 1991, 1992, 1994, 1997, 2002, 2006, 2008, 2012, 2013, 2040, 2042


this PR should be merge before any TM related changes. It should have worked properly ages ago.
Now TM gets reasonable input representations, and therefore "computes properly" how it should in a normal, working setup.

As a result, TM is 3x slower. But that is correct, it computes something now finally.

breznak · 2019-07-12T12:08:11Z

src/examples/hotgym/HelloSPTP.cpp

      };
      goldTM.setSparse(deterministicTM);

-      const float goldAn = 1.0f;
+      const float goldAn = 0.745098f;
+      const float goldAnAvg = 0.408286f;


average anomaly has finaly improved -> TM learns the sequence now 👍

breznak · 2019-07-12T12:13:17Z

src/htm/algorithms/SpatialPooler.cpp

  NTA_CHECK(localAreaDensity > 0.0f && localAreaDensity <= 1.0f);
+  NTA_CHECK(static_cast<UInt>(localAreaDensity * getNumColumns()) > 0) 
+	  << "Too small density or sp.getNumColumns() -> would have zero active output columns.";


This is an important check that caused many SP tests to "break". Ensure sparsity & num columns are such that there will be some >0 desired active columns. (with numActivePerInhArea this was a different issue, for sparsity it had been broken)

TODO followup: bump this number further up, say 40, to ensure some stability and noise robustness of the output SDRs (SDR with 1 active bit is not really good).

I can make this a constant in SP

but actually bumping it would again break many tests that use too trivial, non-realisting settings to demonstrate just the one idea being tested.

breznak · 2019-07-12T12:14:07Z

src/htm/regions/SPRegion.cpp

@@ -290,41 +289,11 @@ Spec *SPRegion::createSpec() {
          "inhibition logic will insure that at most N columns remain ON "
          "within a local inhibition area, where N = localAreaDensity * "
          "(total number of columns in inhibition area). "
-          "Mutually exclusive with numActiveColumnsPerInhArea. "
-          " Default ``-1.0`` which means disabled.",
+	  "Default 0.05 (5%)",


default SP sparsity changed everywhere to 5%

breznak · 2019-07-12T12:16:32Z

src/test/unit/algorithms/SpatialPoolerTest.cpp

+  //As we added SP check that sparsity*numColumns > 0, which is correct requirement.
+  //But many tests have very small (artificial) number of columns (for convenient results),
+  //therefore the check is failing -> we must set high sparsity at initialization. 
+  EXPECT_NO_THROW(sp.initialize(inputDim, columnDim, 16u, 0.5f, true, sparsity));


SP tests now use "setup()" hack to override default sparsity (reasonable, low), to one that does not trigger the SP check by using large(r) sparsity (50%)

breznak · 2019-07-12T12:17:56Z

src/test/unit/algorithms/SpatialPoolerTest.cpp

-    // Test 1D with dimensions of length 1.
-    SpatialPooler sp(
-        /*inputDimensions*/ {1},
-        /*columnDimensions*/ {1});


deleted this dummy test as it's already too artificial. And I even cannot pass sparsity=100% to setup() because we have internal SP check for max sparsity <= 50%.

breznak · 2019-07-12T12:18:40Z

src/test/unit/algorithms/SpatialPoolerTest.cpp

@@ -1660,7 +1670,7 @@ TEST(SpatialPoolerTest, getOverlaps) {
  SpatialPooler sp;
  const vector<UInt> inputDim = {5};
  const vector<UInt> columnDim = {3};
-  sp.initialize(inputDim, columnDim);
+  setup(sp, inputDim, columnDim);


this is example of use of setup. initialize() would throw assert, so we call setup() that overrides sparsity to 0.5f

dkeeney

I have no problem with these changes.
But since I have not been following the discussion very closely I think @ctrl-z-9000-times should be the one to approve this.

ctrl-z-9000-times

Looks good to me!

It's great to see the anomaly of the C++ Hello_SP_TM benchmark go down :)

breznak · 2019-07-13T08:14:05Z

Thank you both for reviewing!

TODO followup: bump this number [of lower bound num columns * sparsity] further up, say 40, to ensure some stability and noise robustness of the output SDRs (SDR with 1 active bit is not really good).

#549 (comment)
what do you think about this point? Add it as hardcoded constant to SP, or as a parameter?

breznak · 2019-07-13T08:20:11Z

BTW, @ctrl-z-9000-times how does parameter optimization deal with asserts?
My point is if asserts can be caught like exceptions, or if we should rather throw exceptions in some places.
Eg if at some point it'd try to set sparsity=0.01 and that is too low (like in the tests). Does it completely kill the one branch of probed combinations, or just notice that and skips the number?

ctrl-z-9000-times · 2019-07-13T16:51:11Z

TODO followup: bump this number [of lower bound num columns * sparsity] further up, say 40, to ensure some stability and noise robustness of the output SDRs (SDR with 1 active bit is not really good).

Well zero active columns is certainly an error, and 1 probably will not work, but the magic minimum number which does work is unclear, and different problems have different requirements. Also many of the unit tests create very small SP's, which could be an issue.

BTW, @ctrl-z-9000-times how does parameter optimization deal with asserts?

It catches them and assumes that it tried invalid parameters, it moves on.

SP: remove numActiveColumnsPerInhArea

7dec66b

and all of its methods. Use `localAreaDensity` instead.

breznak added SP API_BREAKING_CHANGE code code enhancement, optimization, cleanup..programmer stuff labels Jul 9, 2019

breznak requested review from dkeeney and ctrl-z-9000-times July 9, 2019 13:12

breznak self-assigned this Jul 9, 2019

breznak commented Jul 9, 2019

View reviewed changes

breznak mentioned this pull request Jul 9, 2019

SP return const random representation on empty input & SP anomaly #550

Open

3 tasks

ctrl-z-9000-times reviewed Jul 10, 2019

View reviewed changes

src/htm/regions/SPRegion.cpp Show resolved Hide resolved

ctrl-z-9000-times reviewed Jul 10, 2019

View reviewed changes

breznak added 2 commits July 10, 2019 11:08

Merge branch 'master_community' into sp_rm_numActiveColumnsPerInhArea

c8dbcd2

breznak commented Jul 10, 2019

View reviewed changes

breznak added 11 commits July 12, 2019 09:11

Revert "SPRegion: re-enable numActiveColsPerInhArea for compatibility"

735d12b

This reverts commit 4660174.

Merge branch 'master_community' into sp_rm_numActiveColumnsPerInhArea

9fda8e2

SP bump default sparsity to 5%

a58028c

localAreaDensity = 0.05 aka 5%

Hotgym: update deterministic SDRs for new sparsity

22e9eff

fix TMRegion exact output tests

537e076

fix SPRegion tests for removed param

9ecc345

compilation: disable fat LTO for faster builds

d15333b

cmake: use ld.gold flag only if installed

9aa9d0a

cmake: optionally use lld linker if installed

35a2de2

which should have the fastest linking speeds

SP add check for too low sparsity x numColumns combination

eb204f4

when localAreaDensity, aka sparsity is set, check with column sizes that produced active output columns could be > 0

SP fixing tests to match the new added check for sparsity

be812dc

breznak requested a review from ctrl-z-9000-times July 12, 2019 12:03

breznak added the ready label Jul 12, 2019

breznak commented Jul 12, 2019

View reviewed changes

CI fixes for debug

8a64688

dkeeney reviewed Jul 12, 2019

View reviewed changes

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

View reviewed changes

ctrl-z-9000-times merged commit bb4f7a6 into master Jul 12, 2019

ctrl-z-9000-times deleted the sp_rm_numActiveColumnsPerInhArea branch July 12, 2019 23:24

This was referenced Apr 17, 2020

Performance Comparison to nupic #792

Open

Sp reintroduce num active columns per inh area #797

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SP: remove numActiveColumnsPerInhArea #549

SP: remove numActiveColumnsPerInhArea #549

breznak commented Jul 9, 2019

breznak left a comment •

edited by ctrl-z-9000-times

Loading

breznak Jul 9, 2019

ctrl-z-9000-times Jul 10, 2019

breznak Jul 10, 2019

breznak Jul 9, 2019

ctrl-z-9000-times left a comment

breznak left a comment

breznak Jul 10, 2019

breznak Jul 10, 2019

breznak Jul 10, 2019

dkeeney commented Jul 10, 2019

breznak commented Jul 10, 2019

dkeeney commented Jul 10, 2019

breznak commented Jul 10, 2019

dkeeney commented Jul 10, 2019

breznak left a comment

breznak Jul 12, 2019

breznak Jul 12, 2019

breznak Jul 12, 2019

breznak Jul 12, 2019

breznak Jul 12, 2019

breznak Jul 12, 2019

breznak Jul 12, 2019

breznak Jul 12, 2019

dkeeney left a comment

ctrl-z-9000-times left a comment

breznak commented Jul 13, 2019

breznak commented Jul 13, 2019

ctrl-z-9000-times commented Jul 13, 2019

SP: remove numActiveColumnsPerInhArea #549

SP: remove numActiveColumnsPerInhArea #549

Conversation

breznak commented Jul 9, 2019

breznak left a comment • edited by ctrl-z-9000-times Loading

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

ctrl-z-9000-times left a comment

Choose a reason for hiding this comment

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

dkeeney commented Jul 10, 2019

breznak commented Jul 10, 2019

dkeeney commented Jul 10, 2019

breznak commented Jul 10, 2019

dkeeney commented Jul 10, 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

dkeeney left a comment

Choose a reason for hiding this comment

ctrl-z-9000-times left a comment

Choose a reason for hiding this comment

breznak commented Jul 13, 2019

breznak commented Jul 13, 2019

ctrl-z-9000-times commented Jul 13, 2019

breznak left a comment •

edited by ctrl-z-9000-times

Loading