SpatialPooler cleanup #108

breznak · 2018-11-14T02:17:45Z

cleanup SP code
add lots of const qualifiers
add asserts and checks to get/setters
some optimizations

replaces with VectorHelpers.binaryToSparse()

removed definition of round(), must be provided in c++11

- add many ASSERTs - made const methods & params everywhere possible - some code cleanup,optimization

for serialization

breznak · 2018-11-14T08:42:38Z

Restarting stuck CI

- fix deactivation value to 0 - document mutual exclusion (mutex) with numActivePerInhArea - MAX_LOCALAREADENSITY explains 0.5 value

as it is unused and explained some comments for cleanup

breznak · 2018-11-14T11:03:13Z

@dkeeney please review, SP cleanup& optimization

why is it not initialized in initialize()

breznak

Please review
mostly optimization & cleanups.
Please take a look at the mystery with synPermMax on OSX

breznak · 2018-11-14T13:39:27Z

src/nupic/algorithms/Connections.hpp

+  bool operator<=(const Synapse &other) const = delete;
+  bool operator<(const Synapse &other) const = delete;
+  bool operator>=(const Synapse &other) const = delete;
+  bool operator>(const Synapse &other) const = delete;


unrelated to this PR, I just did it when I was there :P

src/nupic/algorithms/SpatialPooler.hpp

breznak · 2018-11-14T13:40:35Z

src/nupic/algorithms/SpatialPooler.hpp


-  void boostOverlaps_(vector<UInt> &overlaps, vector<Real> &boostedOverlaps);
+  void boostOverlaps_(const vector<UInt> &overlaps, vector<Real> &boostedOverlaps) const;


making this much const seemed for better optimization, roughly 10-20% faster here

breznak · 2018-11-14T13:41:40Z

src/nupic/algorithms/SpatialPooler.hpp

-  Real synPermMin_;
-  Real synPermMax_;
+  Real synPermMin_ = 0.0;
+  Real synPermMax_ = 1.0; //TODO set in initialize(), somehow OSX does not set that?!


@dkeeney can you explain this? It's later set in initialize(), Linux OK, but OSX fails w/o this

breznak · 2018-11-14T13:42:53Z

src/test/unit/algorithms/SpatialPoolerTest.cpp

@@ -1578,8 +1581,9 @@ TEST(SpatialPoolerTest, testInitPermConnected) {

 TEST(SpatialPoolerTest, testInitPermNonConnected) {
  SpatialPooler sp;
+  EXPECT_TRUE(sp.getSynPermMax() == 1.0) << sp.getSynPermMax(); 


simple test to debug the synPermMax on OSX, it gave ~0.0

breznak · 2018-11-14T13:45:01Z

src/nupic/algorithms/SpatialPooler.cpp

@@ -416,7 +427,7 @@ void SpatialPooler::initialize(
  synPermMin_ = 0.0;
  synPermMax_ = 1.0;


it should have been set here!!

dkeeney · 2018-11-14T14:43:32Z

src/nupic/algorithms/SpatialPooler.cpp

@@ -1183,7 +1192,7 @@ void SpatialPooler::load(istream &inStream) {
  // Check the saved version.
  UInt version;
  inStream >> version;
-  NTA_CHECK(version <= version_);
+  NTA_CHECK(version == version_);


So, you are not going to be backward compatible with the previous version? I guess that is reasonable.

good spot! Yeah.. here it would be possible to support the old format, but I've decided to prefer clean,small codebase (not many people actually relying on our code now) Do you agree with the removal?

src/nupic/algorithms/SpatialPooler.hpp

dkeeney

Adding range checks for values is a good idea.

There should be lots of opportunities for cleanup here.
Suggest changing all Real's to Real32 so we lock in the size. Same with Int and UInt. For those places where size matters. use size_t (or Size) as type for values that deal with things returned from various size() functions. What we want to do is avoid automatic type conversions.

In many cases these are actually SDR arrays so they should be perhaps Byte arrays but maybe that would be going too far for now.

As long as you are in there, everywhere that there are numeric literals, add an 'f' or 'u' as appropriate for the data type. i.e. 0.05f This will save me the trouble of doing this later. MSVC complains if there is a type mismatch with literals. I also saw places where a literal was cast to a Real; just use the 'f'.

I saw you added a lot of auto's. That is good in that it allows the compiler to optimize more. I personally don't like using auto because I cannot see what the type is but that is just me.

Overall, good job.

dkeeney · 2018-11-14T14:53:44Z

Is there a way to just turn off appveyor for now?

breznak · 2018-11-14T14:56:46Z

Suggest changing all Real's to Real32 so we lock in the size. Same with Int and UInt.

is this an advantage? Would sb want to run on Real(64)?
Anyway,
using Real = Real32; should do, right?

n many cases these are actually SDR arrays so they should be perhaps Byte arrays but maybe that would be going too far for now.

Too far. My plan is to separate into smaller pieces in #92 and then focus on the common SDR-type #109

add an 'f' or 'u' as appropriate for the data type. i.e. 0.05f This will save me the trouble of doing this later

Will do!

because I cannot see what the type is but that is just me.

Yeah, it's person to person, I like it just for that reason, that I don't usually know the type exactly :)

Thanks, will do the suggested changes and merge.

breznak

Some thoughts about the type conversions, what do you think?

breznak · 2018-11-14T16:00:57Z

src/nupic/algorithms/SpatialPooler.hpp

+// force small data types
+using Real = Real32;
+using UInt = UInt32;
+using Int = Int32; 


How is this useful on per file basis? I think the idea of Real was exactly to avoid type conversions, so the whole codebase would use Real (etc) and then in one file you set Real = Real32 for all the code. Isn't that what you want?
Using Real64/32 is only for explicit cases.

Actually, in Types.hpp all Real's are set to Real32. (Unless you set double precision)
But it is not obvious looking at the code how big it is. I personally would like to see Real32 everywhere....or even "float" for floating point numbers.

There are reasons for being 32 bit. One example are the Links. The links between plugins uses a fixed with of 32bits per element. This is 'typeless' because in reality it could be any 32bit type so the buffer is defined as an array of char. Buffer size then is elements * element size. It then does a char array copy. The bigger the size of the element the longer it takes to copy. There are other places that also use byte arrays to hold data of any type.

Since the values are always 0, non-0 in SDR's we could be consistent and use Byte arrays everywhere (for SDR's) and save conversion and copy time. For links that are not SDR's we could still handle because the link is passing ArrayBuffer objects which know the type and length. But for SDR array's we could shorten the copy time by using Byte.

Anyway, that is my soapbox. Perhaps we should defer this for a separate PR and do some research into how best to handle SDR's as has been suggested in #109.

breznak · 2018-11-14T16:03:55Z

src/nupic/algorithms/SpatialPooler.hpp

@@ -1248,8 +1245,8 @@ class SpatialPooler : public Serializable
  bool wrapAround_;
  UInt updatePeriod_;

-  Real synPermMin_;
-  Real synPermMax_;
+  Real synPermMin_ = 0.0f;


and same here,
does MSVC really complain about Real a = 0;? It should be able to infer the exact type at compile-time. That's what all other compilers do.

And if it complains, I think we should do Real a = (Real)0; instead (of = 0.0f) because the (T)cast is more flexible if you decided to change using Real = double you wouldn;t have to change all literals in codebase again.

MSVC does handle 0 ok. But 5.0 is assumed to be a double and if being assigned to a float it is a type conversion.

My personal feeling is that all types should be specific and locked in. CSharp requires it. Python goes the other way and allows anything because values carry type at runtime. But Python pays for that flexibility in execution time.

into sp_cleanup

for C, not C++. reduces compilation messages

set methods are mutex and disable the other automatically

breznak · 2018-11-15T11:41:48Z

@dkeeney ready for round 2 review,
I've managed to turn off the Windows CI (unused)
and fixed API compliance (disabled=-1, thanks for spotting that!)

dkeeney

looks good.

breznak added 12 commits January 18, 2018 10:13

SP cleanup - small

dddc0a6

SP: removes countConnected_()

0717f9e

replaces with VectorHelpers.binaryToSparse()

SP tiny cleanups

976d49d

Merge branch 'master_community' into sp_cleanup

31f0673

SpatialPooler: add clip test

db4872f

SpatialPooler: make more methods const

91a33d1

SpatialPooler: remove hack for MSVC < c++11

2c24440

removed definition of round(), must be provided in c++11

SP: cleanup includes

b7cd0cb

SP make params in constructor, initialize const

763652b

SpatialPooler: big cleanup

e15b110

- add many ASSERTs - made const methods & params everywhere possible - some code cleanup,optimization

SpatialPooler: rm saveFloat_, use normal stream<<

c66f777

for serialization

SP: add loop invariant, avoid while(true)

3365405

breznak added optimization code code enhancement, optimization, cleanup..programmer stuff labels Nov 14, 2018

breznak added this to the optimization milestone Nov 14, 2018

SpatialPooler: rm toDense_(), use VectorHelpers

3515204

breznak self-assigned this Nov 14, 2018

SP: fix tests and checks

833f2f4

breznak requested a review from dkeeney November 14, 2018 03:56

breznak closed this Nov 14, 2018

breznak reopened this Nov 14, 2018

breznak added 2 commits November 14, 2018 10:35

SP: explain localAreaDensity

da9f143

- fix deactivation value to 0 - document mutual exclusion (mutex) with numActivePerInhArea - MAX_LOCALAREADENSITY explains 0.5 value

SP: fix no_throw in tests

8e6e952

breznak mentioned this pull request Nov 14, 2018

Spatial Pooler: separate Inhibition, Topology, Boosting into standalone classes #92

Open

3 tasks

breznak added 3 commits November 14, 2018 11:35

Connections::Synapse: make ordering explicitely deleted

28446ad

as it is unused and explained some comments for cleanup

SP: use NTA_CHECK in public setters

204fa95

SP: finally fix testcase

92dc662

breznak added the ready label Nov 14, 2018

breznak added 2 commits November 14, 2018 13:36

SP debug test for OSX

a9dc836

SP fix synPermMax for OSX

b67243b

why is it not initialized in initialize()

breznak commented Nov 14, 2018

View reviewed changes

dkeeney reviewed Nov 14, 2018

View reviewed changes

src/nupic/algorithms/SpatialPooler.hpp Outdated Show resolved Hide resolved

dkeeney previously approved these changes Nov 14, 2018

View reviewed changes

breznak added 2 commits November 14, 2018 16:02

SP: force Real, UInt, Int templates to 32bit variant

bf475df

SP: explicitely set type for numeric constants

4713299

breznak dismissed dkeeney’s stale review via 4713299 November 14, 2018 15:30

breznak commented Nov 14, 2018

View reviewed changes

breznak and others added 5 commits November 14, 2018 16:28

Merge branch 'master' into sp_cleanup

97a3d15

Merge branch 'master_community' into sp_cleanup

01d0ba3

use typedef instead of c++11 using because of SWIG

0f93466

Merge branch 'sp_cleanup' of https://github.com/htm-community/nupic.core

aef8bb9

into sp_cleanup

Remove unused compiler flag,

3536d45

for C, not C++. reduces compilation messages

breznak closed this Nov 15, 2018

breznak reopened this Nov 15, 2018

breznak closed this Nov 15, 2018

breznak reopened this Nov 15, 2018

breznak closed this Nov 15, 2018

breznak reopened this Nov 15, 2018

SP: fix API: disabled feature uses -1

128ea63

set methods are mutex and disable the other automatically

dkeeney approved these changes Nov 15, 2018

View reviewed changes

dkeeney merged commit 6c41a83 into master Nov 15, 2018

breznak deleted the sp_cleanup branch November 15, 2018 15:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SpatialPooler cleanup #108

SpatialPooler cleanup #108

breznak commented Nov 14, 2018 •

edited

breznak commented Nov 14, 2018

breznak commented Nov 14, 2018

breznak left a comment

breznak Nov 14, 2018

breznak Nov 14, 2018

breznak Nov 14, 2018

breznak Nov 14, 2018

breznak Nov 14, 2018

dkeeney Nov 14, 2018

breznak Nov 14, 2018

dkeeney left a comment

dkeeney commented Nov 14, 2018

breznak commented Nov 14, 2018

breznak left a comment

breznak Nov 14, 2018

dkeeney Nov 14, 2018

breznak Nov 14, 2018

dkeeney Nov 14, 2018

breznak commented Nov 15, 2018

dkeeney left a comment


		void boostOverlaps_(vector<UInt> &overlaps, vector<Real> &boostedOverlaps);
		void boostOverlaps_(const vector<UInt> &overlaps, vector<Real> &boostedOverlaps) const;

		@@ -416,7 +427,7 @@ void SpatialPooler::initialize(
		synPermMin_ = 0.0;
		synPermMax_ = 1.0;

SpatialPooler cleanup #108

SpatialPooler cleanup #108

Conversation

breznak commented Nov 14, 2018 • edited

breznak commented Nov 14, 2018

breznak commented Nov 14, 2018

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

dkeeney left a comment

Choose a reason for hiding this comment

dkeeney commented Nov 14, 2018

breznak commented Nov 14, 2018

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

breznak commented Nov 15, 2018

dkeeney left a comment

Choose a reason for hiding this comment

breznak commented Nov 14, 2018 •

edited