Full sync of C++ temporal memory to Python temporal memory #3254
Conversation
…d added missing function to connections
|
By analyzing the blame information on this pull request, we identified @david-ragazzi, @chetan51 and @scottpurdy to be potential reviewers |
|
|
||
|
|
||
| class SynapseData(object): | ||
| class Segment(object): | ||
| ''' Class containing mininal information to identify a unique segment ''' |
|
@andrewmalta13 @mrcslws Just a small question and hope that the occurrence and usage of the new |
|
@cogmission the GroupByGenerator is an implementation detail. The thing to pay attention to is |
| __slots__ = ['iterable', 'beginning', 'length', 'end', 'index'] | ||
|
|
||
| def __init__(self, iterable, beginning, length): | ||
| self.iterable = iterable |
There was a problem hiding this comment.
Is "iterable" an appropriate name? Iterables don't necessarily support index lookup. Just calling it lis would be good.
There was a problem hiding this comment.
lis? Does not seem like a good name to me. Is it a Python sequence?
| self.activeSegments, segToCol, | ||
| self.matchingSegments, segToCol): | ||
| (column, | ||
| activeColumns, | ||
| activeSegmentsOnCol, | ||
| matchingSegmentsOnCol) = columnData | ||
| if len(activeColumns): | ||
| if len(activeSegmentsOnCol) != 0: | ||
| if not activeColumns is None: |
There was a problem hiding this comment.
For these it's better to do is not None.
I just checked and it's in PEP-8: "Use is not operator rather than not ... is" https://www.python.org/dev/peps/pep-0008/
|
|
||
|
|
||
| def groupby2(*args): | ||
| """ An extension to groupby in itertools. Allows to |
There was a problem hiding this comment.
nit: reformat the paragraph now that this line is narrower
|
I played with the We'll have to make lots of updates to that code if we want it to keep working with this change. I think we should go ahead and copy-paste the old code into |
|
Done reviewing, very nice work! 👍 |
|
👍 |
| generator objects, allowing for groupByN to not have to capture the | ||
| group in a list. | ||
|
|
||
| @param lis (list) the list to perform the groupby on |
There was a problem hiding this comment.
Does this have to be a list? Can it be any sequence type?
There was a problem hiding this comment.
I think you're commenting on old code. The latest is here: https://github.com/numenta/nupic/blob/master/src/nupic/support/group_by.py
There was a problem hiding this comment.
K thx, I was going through old email and saw unresponded-to-comments making me think it was current code.
fixes #3239
fixes #3253
fixes #3240
fixes #3199
fixes #3230
fixes #1511
fixes #2999
fixes #2980
Edit: This change is now ~7-10% faster than the current implementation (about 30% faster than the original phases TM). Compatibility tests pass and fixes a bunch of tm issues in nupic.
Please Review:
@mrcslws @scottpurdy
CC:
@cogmission