Connections::synapsesForPresynapticCell() failed

[Thanh-Binh]

trafficstars

this function does not work well and has the following error like: terminate called after throwing an instance of 'std::out_of_range' what(): Map_base::at because the index presynapticCell is bigger than the map size. Please insert a check before... Thanks #################################################### vector<Synapse> Connections::synapsesForPresynapticCell(const CellIdx presynapticCell) const { const auto& potential = potentialSynapsesForPresynapticCell.at(presynapticCell); const auto& connected = connectedSynapsesForPresynapticCell_.at(presynapticCell);

vector<Synapse> all( potential.cbegin(), potential.cend()); all.insert( all.cend(), connected.cbegin(), connected.cend()); return all; }

[Thanh-Binh]

Here is my version, that works correctly. ############################################################# vector<Synapse> Connections::synapsesForPresynapticCell(const CellIdx presynapticCell) const { vector<Synapse> all; if (potentialSynapsesForPresynapticCell_.count(presynapticCell)) { const auto& potential = potentialSynapsesForPresynapticCell_.at(presynapticCell); all.assign(potential.begin(), potential.end()); }

if (connectedSynapsesForPresynapticCell_.count(presynapticCell)) { const auto& connected = connectedSynapsesForPresynapticCell_.at(presynapticCell); all.insert( all.cend(), connected.cbegin(), connected.cend()); } return all; }

[ctrl-z-9000-times]

Thanks Thanh-Binh,

That's a good fix. If you submit a PR with this change I will approve it.

[breznak]

Thanks for finding the bug and providing a patch! additionally would be great if you can attach a test-case that triggered this

[Thanh-Binh]

@breznak @ctrl-z-9000-times sorry I am so busy so that I can not contribute well in the development. It is very kind of you, all to change it in your process.

Moreover I want to propose to insert some new APIs for Connection-class. I think it is more useful I already implemented and tested by me with nupic.core.

1. create a given number of synapses for a segment connected randomly to a cell set

void createSynapses4Segment(const Segment& segment, UInt32 nSyns, const vector<CellIdx> &Cells, Permanence initPermanence, Random rGen)

2. generate some new synapse->s on the specific segment at presynaptic cells defined by cell candidates

void growSynapses(const Segment& segment, UInt32 nDesiredNewSynapses, const vector<CellIdx>& cellCandidates, Permanence initPermanence, Random random, Int32 maxSynapsesPerSegment)

this function is already used in TM, but I think we can use it in the future for L2, L6a etc. so that it should be in Connection class

3. Compute number of active synapses connecting cells with active presynaptic cells In comparison to the function computeActivity(), which calculates the overlaping at the segment level, this function provides overlaping at the cell level, the length of output vector is number of cells

vector<UInt32> computeActivityForCells(const vector<CellIdx> &activePresynapticCells, Permanence connectedPermanence)

What do you think? Thanks

[breznak]

t I can not contribute well in the development. It is very kind of you, all to change it in your process.

I've submitted a PR with your patch. Thanks again for finding the bug.

Moreover I want to propose to insert some new APIs for Connection-class.

I'm definitely open to extending Connections API but there are a few things to consider:

• the Connections API should provide minimal building blocks
• used commonly by other core algorithms (so no too specific solutions)

So some reasons to add new functions:

• it's faster when implemented in connections
• not easily implemented by existing methods
• used by existing code in this repo
  • we do already have some expections for coming code
  • and I'm starting to feel we should do better and move more specific-only code to its use-classes, minimizing the Connections interface.

Some of my opinions on the proposed changes:

create a given number of synapses for a segment connected randomly to a cell set

void createSynapses4Segment(const Segment& segment, UInt32 nSyns, const vector &Cells, Permanence initPermanence, Random rGen)

• cannot this be relatively easily already achieved?
• the vector& Cells is the "input field" of the segment?
• this could probably be useful for "topology" where synapses initialize only from given area, not the global input space.

generate some new synapse->s on the specific segment at presynaptic cells defined by cell candidates

void growSynapses(const Segment& segment, UInt32 nDesiredNewSynapses, const vector& cellCandidates, Permanence initPermanence, Random random, Int32 maxSynapsesPerSegment)

• this function is already used in TM, but I think we can use it in the future for L2, L6a etc. so that it should be in Connection class

we can use it in the future for L2, L6a etc. so that it should be in Connection class

I'd say when we have other use-case, let's move it, until then..

Compute number of active synapses connecting cells with active presynaptic cells In comparison to the function computeActivity(), which calculates the overlaping at the segment level, this function provides overlaping at the cell level, the length of output vector is number of cells

vector computeActivityForCells(const vector &activePresynapticCells, Permanence connectedPermanence)

this function provides overlaping at the cell level,

do you mean post-synaptic cell here? ie the one containing said segments? Or would you like to compute activity for pre-syn cells, so "how much of the input gets connected"?

• if the former, cannot this already be achieved by looping through the segments on the cell?
  • and it would probably be implemented so in Connections
  • does it make sense to care for cell (num synapses) activity? The segments fulfil a computational role (activationThreshold, the cell operates with num active segments,)

[ctrl-z-9000-times]

sorry I am so busy so that I can not contribute well in the development.

That's ok. It's great that you at least have the time to try out the software and talk to us about it.

2. generate some new synapse->s on the specific segment at presynaptic cells defined by cell candidates

void growSynapses(const Segment& segment, UInt32 nDesiredNewSynapses, const vector& cellCandidates, Permanence initPermanence, Random random, Int32 maxSynapsesPerSegment)

I think this is great idea. As Breznak points out: the existing method TemporalMemory.growSynapses does the same thing. I agree that we should move it to the connections class, since it's necessary for modeling distal dendrites.

I'd say when we have other use-case, let's move it, until then..

I've already duplicated growSynapses in my experimental columnPooler2 branch, and it appears that Than-binh has use this method (outside of the TM) as well.

You other ideas I'm not so sure about. I think that #2 is a more general purpose method than #1. I don't understand the use-case for #3.

[Thanh-Binh]

@breznak growSynapses is already used in the implementation of L6a at htmresearch "ThresholdedGaussian2DLocationModule"

The idea "computeActivityForCells"

vector<UInt32> computeActivityForCells(const vector<CellIdx> &activePresynapticCells, Permanence connectedPermanence)
{
  vector<UInt32> numActiveConnectedSynapsesForCell(numCells(),0);
  for (CellIdx pCell : activePresynapticCells)
  {
    for (Synapse synapse : synapsesForPresynapticCell(pCell))
    {
      const SynapseData &synapseData = dataForSynapse(synapse);
      if (synapseData.permanence >= connectedPermanence - EPSILON)
        ++numActiveConnectedSynapsesForCell[cellForSegment(synapseData.segment)];
    }
  }
  return numActiveConnectedSynapsesForCell;
};

is already used for ColumnPooler at htmresearch. Exactly, it calculates the number of cells supported by feedforward input defined by input vector "activeInputs"

The idea "createSynapses4Segment" is already used some where in the code of @ctrl-z-9000-times. I find it very useful for other implementations of L2 ans L6a.

void createSynapses4Segment(const Segment& segment, UInt32 nSynapses, const vector<CellIdx> &Cells, Permanence initPermanence, Random rGen)
{
  // Pick nSynapses cells randomly from Cells.
  if (nSynapses > Cells.size() || nSynapses < 0) nSynapses = Cells.size();

  vector<CellIdx> candidates(Cells.begin(), Cells.end());
  for (UInt32 c = 0; c < nSynapses; c++)
  {
    size_t i = rGen.getUInt32(candidates.size());
    createSynapse(segment, candidates[i], initPermanence);
    candidates.erase(candidates.begin() + i);
  }
};

[breznak]

ok, first of all, I have an understanding for both approaches. The more useful Connections is, the merrier. From maintanability POV, I have to be careful we don't end up like the old SparseMatrix class, which had methods for everything, nobody knew all its functionality, and we were afraid to modify it.

growSynapses is already used in the implementation of L6a at htmresearch

we are not c++ implementation of htm.research, on the other hand, this method already exists and as you say is useful. And it's low-level enough for modelling dendritic connections. :+1:

"createSynapses4Segment" is already used some where in the code of

in this case I do not think this method offers any new functionality or is complicated enough to justify a special method in Connections(?) I think a Column pooler or other usecases can easily implement it with the code you've shown, or maybe even more readable implementation:

std::vector<int> tmp( 1234 );
std::iota( std::begin( tmp ), std::end( tmp ), 1 );
const auto& candidates = Random.sample(tmp, n); //should we provide an override for sample(nTotal, nCandidates) ? to avoid the 2 lines above, if useful?
for( const auto syn: candidates) {
  createSynapse(seg, syn, perm);
}

So I'd say this one may not be needed. :-1: What do you think?

About

vector<UInt32> computeActivityForCells

Exactly, it calculates the number of cells supported by feedforward input defined by input vector "activeInputs"

I'd have to think more about this one. Would be nice if we could implement equivalent to cellsToColumns(), so segmentsToCells which would tell which cells are active based on activity of segments. Which is basically what you're saing. Or have a parameter {segments, cells, columns} in computeActivity to make a reference what becomes active?

All in all, I'd like us to think more which methods can be moved away from conn, which should be moved in, and which can be adapted to be more useful in general.

[Thanh-Binh]

@breznak I totally agree with you that there is no need to port SparseMatrix into Connection. I found my proposed API for Connections are useful for all, because we use them more often in different implementations currently and in the future. Currently, we have 2 implementations of @ctrl-z-9000-times and me that use those APIs many time. Naturally, we can write them as utils-functions and use them outside of Connections-Class, BUT I do not think it makes our codes clearly.... What do you think?

[ctrl-z-9000-times]

can easily implement it

The implementation you've shown is incomplete and unoptimized. It's not as simple as just a random sample.

• It's missing the steps where you remove duplicate inputs, to prevent an input from forming many synapses to one segment.
• It's missing a call to destroyMinPermanenceSynapses which makes room on the segment for more synapses.
• The implementation of growSynapses in the columnPooler2 research branch is 20 lines long excluding comments, which is a non-trivial amount of C++ code.

From maintanability POV, I have to be careful we don't end up like the old SparseMatrix class

The big problem with the sparse matrix class was that the methods were named with sparse matrix terminology, and there was no documentation for how it was supposed to be used for making synapses. It wasn't just badly written, it was obfuscated and the code was too big to figure out (without any docs).

[breznak]

The implementation of growSynapses

is it the same as in TM? I said I was :+1: for this method to be in Conn.

he implementation [of createSynapses] is incomplete and unoptimized [with missing points for:]

missing the steps where you remove duplicate inputs, to prevent an input from forming many synapses to one segment.

missing a call to destroyMinPermanenceSynapses which makes room on the segment for more synapses.

Ok, I was reacting to the proposed implementation, which did not support either and would therefore be "easily implemented". If we want to do properly the above, I think some (both) of the missing points are implemented in TM (which is currently the only user), it would make sense to have this in Connections.

So how to implement that?

• find an ideal way how to merge this with createSynapse (single)
  • ideally make createSynapse private
  • or add these 2 features to it
  • make an overload for adding the vector of candidates

I still do not understand completely the 3rd proposed method, but we should address each of these as a separate PR/issue.

[breznak]

The implementation of growSynapses in the columnPooler2 research branch

this issue got stuck, but I'd like to highlight growSynapses() which is now used in

• Thalamus (L6) from htm.research by @fcr, implemented in #676
• in ColumnPooler #285
• similar(?) method already in c++ TM.growSynapses_()

So,

• Can we unite their API?
• make a c++ + py bindings implementation in Connections