htm-community / htm.core

Actively developed Hierarchical Temporal Memory (HTM) community fork (continuation) of NuPIC. Implementation for C++ and Python
http://numenta.org
GNU Affero General Public License v3.0
149 stars 74 forks source link

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

Open breznak opened 5 years ago

breznak commented 5 years ago

After #108

ctrl-z-9000-times commented 5 years ago

I think that all of these changes would break API compatibility. That doesnt mean it can't be done, just that it wont be easy.

Another way is subclasses which over ride certain methods. Its not as flexible of a solution but it seems simpler and less disruptive.

breznak commented 5 years ago

Another way is subclasses

Yes, those classes would not be visible to the public API and SP would call them.

AbstractInhibition
InhibitionLocal
InhibitionGlobal
SP { Inhibition = InhibitionLocal }

something like this.

ctrl-z-9000-times commented 5 years ago

If Boosting was refactored into its own class, I would add this boosting function:

I like this function because it has a zero-crossing at activation-frequency = 1 and an asymptote to infinity at activation-frequency = 0.

breznak commented 5 years ago

That sounds even like a good default, eventhough we'd have to do some tests.

Another motivation for this modularity: while testing performance impact of PR #119 , I figured there are no tests for using local inhibition, at all! The separate class would make that more obvious.

I was thinking about your comment how to make this less intrusive, API-wise. SpatialPooler(..., inhibition*=nullptr) would allow us to create instance of Inhibition and provide it to override the default.

breznak commented 5 years ago

@ctrl-z-9000-times now that David is working on pybind, do you want to join me here and then to #93? I figured we should proceed here first as it separates the SP into smaller logical blocks, where none of Boosting,Inhibition,Topology are used in TM, therefore shielding them will make the port simpler.

ctrl-z-9000-times commented 5 years ago

I would rather work on #93 first. It seems more straightforward than this one.

ctrl-z-9000-times commented 5 years ago

Here are my thoughts on each of these topics:

Boosting: I would like to use the logarithmic boosting function as a default. Maybe we could sneak this in by turning it on when boosting-strength == -1 ?

Topology: Conceptually, this is the function topology( mini-column-location ) -> potential-pool. The SP could keep a pointer to this function, with setter/getter methods, to allow other ppl to hook in their own topology. This allows the defaults to remain, and for new functions to be added. Python function callbacks can be added.

I would add the topology function:

Topology( X )
    RandomSample( num = potential_pool_size
    data = for Input in InputSpace:
                   Normal(loc = Input, mean = X, Std = Radius)

This is the simpler 1-D case. The N-Dimensional extension of this function samples from the product of the normal-distributions in all dimensions. This has the feature of rotational symmetry, which is nice.

Update: I tried this topology function and it didn't perform as well as the one that's already in the spatial pooler. I tested it on the MNIST dataset.

Inhibition: This is a core part of the SP, and I don't think its going to be easy to change them without modifying the whole class. Local inhibition currently is a lot of the code in the SP, and my solution also changes the API.

ctrl-z-9000-times commented 5 years ago

@breznak , I have a proposal for how to implement topology.

We should change the private method: vector<UInt> SpatialPooler::initMapPotential_(UInt column, bool wrapAround);

Into protected method: virtual SpatialPooler::initMapPotential( SDR &column, SDR &potentialPool );

Subclasses of SpatialPooler can then override the default topology.

breznak commented 5 years ago

Subclasses of SpatialPooler can then override the default topology.

This does sound as a good approach! :+1:

I usually tend to prefer a composition solution (Topology class and a member is passed/created, instead of overriding in classes). My reasoning for this is combinatorial explosion - if you want Topology: Wrapping, NonWrapping,Whatnot... AND Inhibition: Local,Global,YourLocalMacroCols,... you either end up with a million overriding classes, the composition let's the user to mix their salad :green_salad: . What do you think?

Another advantage is it makes the main (SP) class leaner by hiding some varables in Topology.

Both the mini-column index and the input index are changed into SDRs for convenience.

Does this help, or is it more confusing? Such SDR contains exactly one ON-bit, the column, right?

ctrl-z-9000-times commented 5 years ago

Do [SDR's] help, or is it more confusing? Such SDR contains exactly one ON-bit, the column, right?

I think SDR's are the way to go:

breznak commented 5 years ago

If I give you a UInt it could mean a lot of things, but an SDR has a very specific meaning.

Agreed, SDR is the way to go here.

I was thinking about adding more vectorization (by means of "batch learning", ...) to the code, so instead of UInt, the parameter would be vector (even if size()==1), but/and SDRs would allow this behavior too.

ctrl-z-9000-times commented 5 years ago

I usually tend to prefer a composition solution [over subclass solution]

I understand this, I just think it's easier to use subclasses until the # of subclasses gets out of hand. For example if topology was an instance of a class which was passed in, then the user would need to create the topology object every time they create an SP, which isn't an easy default. Subclasses also allow the topology code to peek at the SP's protected data, which is convenient and sometimes necessary (especially for boosting)

breznak commented 5 years ago

easier to use subclasses until the # of subclasses gets out of hand

and that's my worry, with topology (2x: Wrapping, Normal); inhibition (3x: Global, Local, MacroColumns); Boosting (2x: Numenta, "Yours") -> 12 combinations. Not that we'd test them all anyway (probably), but parametric approach makes the individual composition easier for the user (and harder for the initial developer). Good example is parameter optimization.

create the topology object every time they create an SP, which isn't an easy default

That's why I propose a "factory":

auto sp = SP(.., string topologyType="normal" /* "normal", "wrapping", etc*/, ....); 

where

SP 
  private: 
    TopologyAbstract topology;
initialize() {
 if(topologyType == "normal") { 
    topology = NormalTopology(...);
} else {...}

Subclasses also allow the topology code to peek at the SP's protected data, which is convenient and sometimes necessary (especially for boosting)

True, use it when it's necessary. On the other hand, it hides topic-related stuff into the "submodule": class Topology, Boosting, Inhibition...

So in the end, it could look like SP(.., "topologyNormal", "boostingLogaritmic", "inhibitionLocal");

What do you think?

ctrl-z-9000-times commented 5 years ago

topology (2x: Wrapping, Normal); inhibition (3x: Global, Local, MacroColumns); Boosting (2x: Numenta, "Yours")

I think you overestimated this:

Topology wrapping is an argument, both versions are implemented by the same code. Also topology has 3 arguments which the factory would need.

Local inhib is slow, has no unit tests, and probably shouldn't be used. My new inhibition subclass can do both local and global inhib.

There are two variants of boosting, which the user might want to compare. Or we could do the comparison and give the user just the better if the two, assuming that one is unambiguously better than the other.

ctrl-z-9000-times commented 5 years ago

On the topic of using SDRs hold the potential pools:

This could be useful for debugging a custom topology, using the sdr metrics classes. Sparsity is the number of potential connections to each output, and active freq is the number of pot syns per input. Set period >= #outputs for true average instead of exp-rolling avg.

If min-sparsity is zero then an output has no synapses. If min-AF is zero then input has no synapses.

This wont actually work because the caller (SP) would need to setup the metrics class, but this is something i hope to improve upon in the metrics classes. Edit: I added this feature to the SDR Metrics classes.

breznak commented 5 years ago

Local inhib is slow, has no unit tests, and probably shouldn't be used. My new inhibition subclass can do both local and global inhib.

The lack of test and speed is true, so you intend this (new inh) to eventually replace the other two? That's a bold idea, in a good way. I'll dig some more into the new inhibition..

ctrl-z-9000-times commented 5 years ago

Hi @breznak,

I'm starting to write the Column Pooler (see issue #94) and since it's new code we can make a new API for the class. For Topology this is what I have so far:

typedef function<void(SDR& cell, SDR& potentialPool)> Topology_t;
...
ColumnPooler(... , Topology_t potentialPool, ...) {
...
class DefaultTopology : public Topology_t
{
  ...
  DefaultTopology(Real potentialPct, Real radius, bool wrapAround)
  : potentialPct(potentialPct), potentialRadius(radius), wrapAround(wrapAround) 
  ...
  void operator()(SDR& cell, SDR& potentialPool) {

In MNIST_SP:

  ColumnPooler htm(
      ...
      /* potentialPool */      DefaultTopology(.9, 4., false),
Thanh-Binh commented 5 years ago

Could you please explain me why topology is needed here? Thanks

ctrl-z-9000-times commented 5 years ago

@Thanh-Binh: This is the topology between the feed forward inputs and the proximal dendrites. It is very much like the spatial pooler's topology, except that instead of allowing a spatial pooler to cover a large topological area, it allows the column pooler to do the same.

Thanh-Binh commented 5 years ago

@ctrl-z-9000-times thanks for your explanation ..

breznak commented 5 years ago

I think this issue gains importance given aim for parallelization #255 , and recent development of ColumnPooler (SP, CP, share inhibition, topology, ...) Extracting them to separate classes would help to drive common interface.