Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Connections better use typedefed types Segment, Synapse, ... #331

Merged
merged 57 commits into from Apr 6, 2019

Conversation

Projects
None yet
3 participants
@breznak
Copy link
Member

commented Mar 18, 2019

  • Connections (and related SP, TM) properly use typedefed types for Segment, SynapseIdx etc
    • allowing us to really change them as needed
  • minor cleanups

Implements size_t for TM, SP, Connections #357

breznak added some commits Mar 16, 2019

Connections: updateSynapsePermanence optimization
performance critical method, make some variables const
Connections::updateSynapsePermanence optimization
remove bounds check for Permanence range, callers must ensure it's
within [minPermanence, maxPermanence]
VectorHelpers: union, sparse_cellsToColumns use template T
allow to be templated, not hardcoded to UInt
Connections: use typedefs Synapse, Segment
instead of UInt,
This allows later changing easily the datatype of said typedef
Connections, SP, TM: use typedefs from Connections
for Synapse, Segment, ... instead of UInt only

@breznak breznak added the code label Mar 18, 2019

@breznak breznak self-assigned this Mar 18, 2019

@breznak
Copy link
Member Author

left a comment

Please have a look and check if I correctly replace with the new types.

@@ -303,10 +306,9 @@ void Connections::updateSynapsePermanence(Synapse synapse,
potentialPreseg.push_back( segment );
}

for (auto h : eventHandlers_) {
for (auto h : eventHandlers_) { //TODO handle callbacks in performance-critical method only in Debug?

This comment has been minimized.

Copy link
@breznak

breznak Mar 18, 2019

Author Member

this method is a hotspot I found in profiling, do we really need the callbacks here, can be left out only for debug,.. ?

This comment has been minimized.

Copy link
@ctrl-z-9000-times

ctrl-z-9000-times Mar 19, 2019

Yea I have no idea why these exist but their part of the public API so I didn't remove them.
Maybe try: for( const auto & : eventHandlers_) for speed?

This comment has been minimized.

Copy link
@breznak

breznak Mar 19, 2019

Author Member

I'll profile w/o the loop, imo this would be a candidate to break the API, if shows performance gain.

typedef UInt16 SynapseIdx; /** Index of synapse in segment. */
typedef UInt32 Segment; /** Index of segment's data. */
typedef UInt32 Synapse; /** Index of synapse's data. */
typedef UInt32 CellIdx; //TODO instead of typedefs, use templates for proper type-checking

This comment has been minimized.

Copy link
@breznak

breznak Mar 18, 2019

Author Member

typedefs have the disadvantage of only renaming the type for the user(=programmer), for proper type-checking (so that compiler checks difference for SegmentIdx, SynapseIdx) we'd need to use templates instead.
I'll consider that in subsequent PR.

Show resolved Hide resolved src/nupic/algorithms/Connections.hpp Outdated
typedef unsigned char SegmentIdx; /** Index of segment in cell. */
typedef unsigned char SynapseIdx; /** Index of synapse in segment. */
typedef UInt16 Segment; /** Index of segment's data. */
typedef UInt16 Synapse; /** Index of synapse's data. */

This comment has been minimized.

Copy link
@breznak

breznak Mar 18, 2019

Author Member

note, a WIP commit changes these to much lower values.
I'll need to asses the impact, suprisingly, the lower values (compared to UInt) do not gain a significant performance gain (possibly some forgotten type-casting happens?)

Unit test SP:: ExactOutput "breaks" as stays stuck, does not finish, nor fail. I don;t know why

This comment has been minimized.

Copy link
@ctrl-z-9000-times

ctrl-z-9000-times Mar 18, 2019

The size of this data type controls the maximum number of synapses & segments.
So with an 8-bit integer, it can only have a 256 synapses, maximum.
16-bits as shown here allows a maximum of 65,536 synapses.

I will give this a more thorough review within a day or two.

This comment has been minimized.

Copy link
@breznak

breznak Mar 19, 2019

Author Member

So with an 8-bit integer, it can only have a 256 synapses, maximum.
16-bits as shown here allows a maximum of 65,536 synapses.

yes. Are you suggesting the 16b should match the 8-bit? As SynapseIdx is "synapses per segment", and Synapse is a flat array/index to all existing synapses (their data). But there can be a number of segments in cells, so Synapses >> SynapseIdx.

This comment has been minimized.

Copy link
@ctrl-z-9000-times

ctrl-z-9000-times Mar 19, 2019

Are you suggesting

No i just wanted to clarify.
What u have here looks good, as long as it doesnt overflow.

This comment has been minimized.

Copy link
@ctrl-z-9000-times

ctrl-z-9000-times Mar 19, 2019

And if it does overflow then we can easily fix it by changing these typedefs which after this PR should be used much more consistently. :)

@breznak breznak referenced this pull request Apr 4, 2019

Closed

Anomaly removal, cleanup #347

0 of 6 tasks complete
@ctrl-z-9000-times
Copy link

left a comment

This looks good to me.

breznak added some commits Apr 4, 2019

SDR: use using for typedef
rather that typedef
SDR: add typedefs for ElemSparse, ElemDense
used in SDR_sparse_t, SDR_dense_t containers (vectors)
ElemSparse must match with connections::CellIdx
ElemDense : allow changing the type in the future
COnnections: bind CellIdx with sdr::ElemSparse
as these must be the same for some comparisons in SP, TM
@breznak
Copy link
Member Author

left a comment

One more review, please.

  • I've merged master
  • implemented "size_t" for Connections,TM,SP which was needed for interchanging the connections' typedefs
  • binded SDR sparse elem type & Connections CellIdx
typedef UInt32 Synapse; /** Index of synapse's data. */
typedef Real32 Permanence;
//TODO instead of typedefs, use templates for proper type-checking?
using CellIdx = nupic::sdr::ElemSparse; // CellIdx must match with sdr::ElemSparse, to change, change it there

This comment has been minimized.

Copy link
@breznak

breznak Apr 4, 2019

Author Member

FYI I had to bind connections::CellIdx and sdr::ElemSparse, where ElemSparse is the element type of SDR_sparse_t (currently UInt32);
Both have to be the same for some methods in SP, TM, so I binded them together. It could be untangeled but then I'd have to do type-conversion of the vectors in said methods.

@@ -333,7 +335,7 @@ class Connections : public Serializable
*
* @retval A vector length
*/
UInt32 segmentFlatListLength() const;
size_t segmentFlatListLength() const { return segments_.size(); };

This comment has been minimized.

Copy link
@breznak

breznak Apr 4, 2019

Author Member

implemented size_t for Connections, TM, and SP.

typedef std::vector<std::vector<UInt>> SDR_coordinate_t;
typedef std::function<void()> SDR_callback_t;
using ElemDense = Byte; //TODO allow changing this
using ElemSparse = UInt32; //must match with connections::CellIdx

This comment has been minimized.

Copy link
@breznak

breznak Apr 4, 2019

Author Member

ElemSparse & ElemDense represent data type of elements in SDR_sparse/dense_t;

  • ElemSparse is used to match in Connections with CellIdx
  • ElemDense is in preparation for changing this (at runtime with teplate?)

@breznak breznak requested a review from ctrl-z-9000-times Apr 4, 2019

@breznak

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

@dkeeney @ctrl-z-9000-times please have a look when you have time, I'd like to continue tomorrow and it'd help to have this already in.

@ctrl-z-9000-times
Copy link

left a comment

Here are some tips for getting PRs merged.

  • Make smaller PRs. The size_t changes probably could have had their own PR, and then most of this work would have been merged a day ago.
  • Don't leave so many TODO notes in the code. Either do them, or talk about them on this github forum. Every change in the code is fair game for us to argue over, and it can really slow things down. If it's a comment of the github page then we can discuss it without blocking the PR from merging.
std::vector<SynapseIdx> &numActivePotentialSynapsesForSegment,
const CellIdx activePresynapticCell,
const Permanence connectedPermanence) const;
void computeActivity(std::vector<SynapseIdx> &numActiveConnectedSynapsesForSegment, //TODO remove the 2 arg version, use only the 3 arg (with 1 being optional)

This comment has been minimized.

Copy link
@ctrl-z-9000-times

ctrl-z-9000-times Apr 5, 2019

No can do, the two different versions do different things. One computes less than have as much as the other, making it significantly faster. Removing it would likely double the spatial poolers runtime.

This comment has been minimized.

Copy link
@breznak

breznak Apr 5, 2019

Author Member

One computes less than have as much as the other, making it significantly faster.

I'm aware, if we use {} the loop will skip, or some empheral value, as in TM(?)

This comment has been minimized.

Copy link
@breznak

breznak Apr 5, 2019

Author Member

/**
* Gets the number of segments.
*
* @retval Number of segments.
*/
Segment numSegments() const;
size_t numSegments() const {
NTA_ASSERT(segments_.size() >= destroyedSegments_.size()); //TODO remove destroyedSegments_?

This comment has been minimized.

Copy link
@ctrl-z-9000-times

ctrl-z-9000-times Apr 5, 2019

This assert is not needed.
destroyedSegments_ is very much needed, otherwise segments could never be destroyed.

This comment has been minimized.

Copy link
@breznak

breznak Apr 5, 2019

Author Member
  • can I keep the assert?, I guess someone can make a working code-change where #destroyed > #segments_

One computes less than have as much as the other, making it significantly faster.

can we just rm the SegmentIdx from the list of segments?

This comment has been minimized.

Copy link
@ctrl-z-9000-times

ctrl-z-9000-times Apr 5, 2019

can we just rm the SegmentIdx from the list of segments?

No, because then the other segment indexes would change. We could go through the work of updating these references since they're mostly contained within the connections class, but I'm not sure it would be any faster.


/**
* Gets the number of synapses.
*
* @retval Number of synapses.
*/
Synapse numSynapses() const;
size_t numSynapses() const {
NTA_ASSERT(synapses_.size() >= destroyedSynapses_.size());

This comment has been minimized.

Copy link
@ctrl-z-9000-times

ctrl-z-9000-times Apr 5, 2019

nit: assert not needed.

bool operator==(const Connections &other) const;
bool operator!=(const Connections &other) const;
virtual bool operator==(const Connections &other) const;
inline bool operator!=(const Connections &other) const { return !operator==(other); }

This comment has been minimized.

Copy link
@ctrl-z-9000-times

ctrl-z-9000-times Apr 5, 2019

virtual is not needed, since this class should never be subclassed. It's not supported, this is the only virtual method here. I expect if it were subclassed that a lot of things would break.

My understanding is that all modern compilers ignore inline and figuring out what to inline on its own.

This comment has been minimized.

Copy link
@breznak

breznak Apr 5, 2019

Author Member

I'll remove the virtual, you're right about the inline, but it does not hurt either.

This comment has been minimized.

Copy link
@ctrl-z-9000-times

ctrl-z-9000-times Apr 5, 2019

inline is a nitpick, you can ignore me here.

connections.createSynapse(segment, candidates[i], initialPermanence);
for (size_t c = 0; c < nActualWithMax; c++) {
const auto i = rng.getUInt32(static_cast<UInt32>(candidates.size()));
connections.createSynapse(segment, candidates[i], initialPermanence); //TODO createSynapse create a vector of new synapses at once

This comment has been minimized.

Copy link
@ctrl-z-9000-times

ctrl-z-9000-times Apr 5, 2019

I disagree with this TODO note. I don't see any advantage to making a list of synapses and then making them all at once. It's more work & complexity to make, populate, and then iterate over an extra list. I think the performance of this issue is likely insignificant either way.

The next line however is almost certainly a performance issue:
candidates.erase(candidates.begin() + i);

This comment has been minimized.

Copy link
@breznak

breznak Apr 5, 2019

Author Member

I don't see any advantage to making a list of synapses and then making them all at once.

I tend to prefer passing data in vectors as long as possible, it avoids nested loops and is better for spotting hot-spots. Also SIMD (auto)vectorization can do a better job then.

The next line however is almost certainly a performance issue:
candidates.erase(candidates.begin() + i);

good point!

Anyway, out of scope of the PR, updated the comments and I'll open a separate issue.

This comment has been minimized.

Copy link
@breznak

breznak Apr 5, 2019

Author Member
Show resolved Hide resolved src/nupic/algorithms/TemporalMemory.cpp
VectorHelpers::unionOfVectors<UInt>(sdr.getSparse(), active, predictive);
sdr.setSparse(sdr.getSparse()); // to update the cache in SDR.
vector<CellIdx> uni;
VectorHelpers::unionOfVectors<CellIdx>(uni, active, predictive);

This comment has been minimized.

Copy link
@ctrl-z-9000-times

ctrl-z-9000-times Apr 5, 2019

Before this was an in-place update and now it makes a new vector and swaps it in...

This comment has been minimized.

Copy link
@breznak

breznak Apr 5, 2019

Author Member

Good spot!
This was for mismatch in SDR_sparse_t, and vector //you can change CellIdx;
but got fixed later by binding connections::CellIdx and sdr::ElemSparse

TL;DR reverted back

@breznak

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

ake smaller PRs. The size_t changes probably could have had their own PR, and then most of this work would have been merged a day ago.

  • you're right, this PR grew big, justification for adding size_t is that it's needed for the typedef changes to reliably work
    • is it OK to review as is, or do I need to split?

Don't leave so many TODO notes in the code. Either do them, or talk about them on this github forum. Every change in the code is fair game for us to argue over, and it can really slow things down. If it's a comment of the github page then we can discuss it without blocking the PR from merging.

this is true, I've abused the code for discussion here, I'll rm the PRs or make them to issues.

@ctrl-z-9000-times

This comment has been minimized.

Copy link

commented Apr 5, 2019

is it OK to review as is, or do I need to split?

This is fine to review as it is, especially since I've already done the review. I just meant in general. Thanks!

@breznak breznak referenced this pull request Apr 5, 2019

Open

TM:growSynapses improve methods #373

0 of 3 tasks complete
@breznak

This comment has been minimized.

Copy link
Member Author

commented Apr 5, 2019

@ctrl-z-9000-times I hope to have addressed your feedback, one more review, please

@breznak breznak requested a review from ctrl-z-9000-times Apr 5, 2019

@ctrl-z-9000-times
Copy link

left a comment

This looks great to me!

@breznak breznak merged commit fcfe07d into master Apr 6, 2019

3 checks passed

ci/circleci: build-and-test Your tests passed on CircleCI!
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@breznak breznak deleted the connections_opt branch Apr 6, 2019

@breznak

This comment has been minimized.

Copy link
Member Author

commented Apr 6, 2019

This looks great to me!

thanks for bearning with me on this PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.