TM: cellsToColumns #397

breznak · 2019-04-11T17:06:07Z

TM now provides cellsToColumns
- removed from VectorHelpers, as the transformation is only valid for TM's cells
~~TM::getOutputColumns convenience method~~
- replaces code in Hotgym + TMRegion
- decided that it's actually incorrect code!
- see TMRegion: allow output as columns, not only cells #413 TMRegion: also output predictiveCells #412
hotgym example uses SDR
deterministic checks for encoder, SP, TM, anomaly; For Deterministic Algorithms #194 , followup TODO
removes unused code from VectorHelpers
- in preparation for removal of that class (sparseToBinary being only one used now)

that SDR.size matches TM.numberOfColumns

which is a convenience wrapper for TM::columnForCell()

these methods have been replaced by TM.cellsToColumns. Because the conversion method is only valid for TM's cells.

add check + print

which represents TM's output as mini-columns, and is a union of active and predictive at the current time. Used by Hotgym example, and TMRegion

src/nupic/algorithms/TemporalMemory.cpp

breznak · 2019-04-11T17:08:07Z

src/nupic/algorithms/TemporalMemory.hpp

+ *
+ * @return SDR size(COLS) 
+ */
+ sdr::SDR getOutputColumns() const;


better name?

I dont think that this is right. I dont think the TM should ever use the union of active & predicted cells.

There are two use cases for converting from cells to minicolumns, decode and anomaly. We've decided not to implement decode. I would really like to see anomaly as part of this PR.

Which of the 2 methods do you objectify to?

cellsToColumns:

yes, a convenience method, columnForCell(cell) already exists

I propose removal of columnForCell

or either of them can be made private

this method is needed for TM::anomaly() which operates with columns.

we had the same functionality in VectorHelpers::cellsToColumns

but I decided to move it to TM, as it's the only user and the way of conversion (plain topology) makes sense only for TM. So as not to make it confusing and tempting for other, misused usages (if in VectorHelpers)

getOutputColumns ..TM output as union of active + predictive columns

conceptually, I think TM should output only columns (not cells)

as cols are needed for it to be used in a hierarchy (and in anomaly, decode,...)

what is Classifier using? It should probably use cols as well.

cells should be an implementation detail and hidden from the user

cells are still accessible for the likes of CP, etc.

I think we should provide all 3 of them:

getColumnsActive(), Predictive(), Both()

(both = current getOutputCols)

or fill them in compute method TM::compute(const SDR& input, bool learn, SDR& output)

impl details how to well decide which mode to fill in output (enum?), default param, etc...

Anomaly, NetworkAPI (TMRegion), Classifier(?) would work with these outputs

I object to getOutputColumns. It should not mix the active & predictive cells together.

It should not mix the active & predictive cells together.

why should not? A TM's minicolumn is activated by at least one active cell, be it feed forward activation, or predictive/contextual.

It's not like it's impossible to compute manually, we have access to the active, predictive cells; and will now have the cellsToColumns transformation.

but the user (for hierarchy, classifier, TMRegion, TM's direct output) has to do exactly what is this method about.

ok, I quickly double checked with https://numenta.com/assets/pdf/biological-and-machine-intelligence/BaMI-Temporal-Memory.pdf
And you're right, there's no sense in mixing active+pred columns.

I'll remove getOutpuColumns

TMRegion outputs that with option orOutputCols, I then say we should rm that too.

instead, do we want to offer TM.activeCols, TM.predictiveCols ? That would be of use for anomaly

@ctrl-z-9000-times ok, we agreed that mixing active + predictive cols/cells does not make sense. I'm removing the method from TM, not looking at TMRegion.
@dkeeney is it necessary for a Region to implement "bottomUpOutput"?

Currently we return either a union of cells (act+pred) or as columns, depending on a switch.

what should we return as bottom up output? (I'd say feed-forward activation as columns)

or should we remove the field altogether? (ideally)

currently, NetworkAPI has now way to obtain columns (active, predicted; separately), and it probably should.

@dkeeney is it necessary for a Region to implement "bottomUpOutput"?

This is its only output. I think we need something. I think we could go with columns only. We could add additional outputs for active and predicted as separate outputs.
What makes since?

any combination of {active, predictive} x {cells, columns} makes sense.
I'll probably open a new PR for that, and just remove the current orOutputColumns logic

See #412 #413

src/nupic/algorithms/TemporalMemory.hpp

src/nupic/regions/TMRegion.cpp

breznak · 2019-04-11T17:11:54Z

src/nupic/utils/VectorHelpers.hpp

- * columns(binary vector). If any cell of a column is active (1), the column
- * is considered active. See TP for details.
- */
- static std::vector<UInt> cellsToColumns(const std::vector<UInt>& cellsBinary, const UInt cellsPerColumn)


moved to TM

ctrl-z-9000-times · 2019-04-11T21:45:12Z

There are two use cases for converting from cells to minicolumns, decode and anomaly. We've decided not to implement decode. I would really like to see anomaly as part of this PR. Please demonstrate how the user will interact with the TM to get the anomaly.

ctrl-z-9000-times · 2019-04-11T22:00:53Z

Ok I read the unit tests and i think i understand how to use it. Would still like to see anomaly though, even if in psuedo code

src/nupic/algorithms/TemporalMemory.cpp

breznak · 2019-04-12T06:59:15Z

Would still like to see anomaly though, even if in psuedo code

I tried to make smaller PRs :) Do you want to keep this separate, or add anomaly here?
Design:

TM {
  struct {
      float score;
      { //hide these
      SDR& prevPredCols;
      SDR& currentActiveCols;
      }
  } anomaly;

  TM::compute() {
    //...do all the stuff
    //for anomaly:
   //TODO Q: How to configure if anom should be done? a) always, b) TM(.., bool doAnomaly), c) getAnomalyScore() does not work that well, as we need to get predictiveColumns, which is expensive. I prefer B. 
  if(doAnomaly_) {
   anomaly.currentActiveCols = tm.cellsToColumns(active_); //or use active/pred cols, if the methods are available
   anomaly.score = computeRawAnomalyScore(activeCols, prevCols); //I'd keep this method in separate file 
   anomaly.prevPredCols = tm.cellsToColumns(getPredictiveCells())
  }
  } //-end TM::compute
}


// usage
TM tm(doAnomaly);
tm.compute(...);
if tm.anomaly.score > 0.9 { //run! }

ctrl-z-9000-times · 2019-04-12T13:52:49Z

I tried to make smaller PRs :) Do you want to keep this separate, or add anomaly here?

Sorry for the conflicting advice. This PR can be just for cellsToColumns and anomaly can come later.

ctrl-z-9000-times · 2019-04-12T16:09:57Z

TODO Q: How to configure if anom should be done?
a) always,
b) TM(.., bool doAnomaly),
c) getAnomalyScore() does not work that well, as we need to get predictiveColumns, which is expensive. I prefer B.

I prefer A, but B is acceptable too.

it does not make sense to mix feed-forward (active) activations with contextual (predictive)

as not supported, follows removal of TM::getColumnsOutput

there's no bug in Random, we're ok!

instead of Scalar

use seed for RDSE, SDR use addNoise instead of randomize

ctrl-z-9000-times

I have a few comments and change requests. Overall this PR is good.

src/examples/hotgym/HelloSPTP.cpp

src/nupic/algorithms/TemporalMemory.cpp

src/nupic/algorithms/SpatialPooler.cpp

to be used for deterministic check and output

outSP SDR has to be initialized with dimensions, even if later successfully assigned (outSP = outSPglobal)

for SP local, also remove the time check for Windows CI

breznak · 2019-04-23T12:41:46Z

Added your feedback, this is now working. Ready for another round of reviews when you got time. Thanks

breznak · 2019-04-23T15:14:34Z

[----------] 1 test from HelloSPTPTest
[ RUN ] HelloSPTPTest.performance
starting test. DIM_INPUT=1000, DIM=2048, CELLS=8
EPOCHS = 5000
starting: 5000 iterations.Epoch = 4999
Anomaly = 0.940001
SP (g)= SDR( 2048 ) 72, 75, 284, 303, 305, 317, 329, 525, 1095, 2027
SP (l)= SDR( 2048 ) 6, 12, 26, 57, 63, 72, 75, 76, 77, 80, 82, 103, 105, 124, 135, 154, 171, 174, 175, 185, 192, 193, 195, 198, 263, 284, 296, 302, 303, 305, 313, 317, 319, 320, 356, 363, 364, 401, 403, 404, 410, 413, 425, 426, 428, 449, 491, 496, 511, 515, 516, 518, 520, 525, 529, 536, 550, 556, 574, 583, 592, 597, 598, 603, 609, 622, 626, 636, 645, 652, 704, 706, 722, 726, 727, 728, 729, 747, 751, 766, 779, 808, 833, 837, 838, 840, 848, 850, 853, 860, 908, 912, 918, 919, 923, 927, 929, 930, 931, 932, 970, 989, 1006, 1038, 1066, 1082, 1085, 1087, 1092, 1094, 1095, 1113, 1115, 1125, 1128, 1174, 1179, 1180, 1182, 1185, 1205, 1206, 1232, 1236, 1238, 1239, 1240, 1245, 1271, 1292, 1295, 1300, 1303, 1307, 1311, 1319, 1320, 1322, 1382, 1401, 1412, 1415, 1421, 1426, 1431, 1434, 1438, 1470, 1474, 1492, 1501, 1511, 1521, 1524, 1525, 1530, 1532, 1537, 1540, 1600, 1617, 1620, 1622, 1632, 1638, 1641, 1667, 1672, 1680, 1684, 1686, 1690, 1699, 1702, 1742, 1744, 1745, 1746, 1765, 1770, 1774, 1801, 1807, 1808, 1816, 1830, 1834, 1849, 1861, 1867, 1871, 1882, 1902, 1907, 1943, 1945, 1955, 1956, 1966, 1968, 1969, 1971, 1986, 2018, 2025, 2027
TM= SDR( 2048 ) 26, 75, 525
==============TIMERS============
Init: 0.0932824
Random: 0.211816
Encode: 0.0174722
SP (l): 39.4186
SP (g): 1.45276
TM: 1.43883
AN: 0.0270594
AN: 0.677287
ERR: CHECK FAILED: "outTM == goldTM" Deterministic output of TM failed!
SDR( 2048 ) 26, 75, 525
should be:
SDR( 2048 ) 26, 75
[c:\projects\nupic-cpp\src\examples\hotgym\hellosptp.cpp line 203]
unknown file: error: C++ exception with description "CHECK FAILED: "outTM == goldTM" Deterministic output of TM failed!
SDR( 2048 ) 26, 75, 525
should be:
SDR( 2048 ) 26, 75
" thrown in the test body.
[ FAILED ] HelloSPTPTest.performance (43378 ms)
[----------] 1 test from HelloSPTPTest (43378 ms total)

I'm not sure why the Windows run would fail deterministic checks .. :(

on Windows, fix later

dkeeney

Ok, this looks good to me.
It looks like the CI passed unit tests so I assume you figured out the problem with Windows. 👍

dkeeney · 2019-04-24T22:35:57Z

src/examples/hotgym/HelloSPTP.hpp

@@ -20,8 +20,8 @@ class BenchmarkHotgym {
 bool useSPglobal=true,
 bool useTM=true,
 const UInt COLS = 2048, // number of columns in SP, TP
- const UInt DIM_INPUT = 10000,
- const UInt CELLS = 10 // cells per column in TP
+ const UInt DIM_INPUT = 1000,


Oh, good. You reduced the size of the test :)

dkeeney · 2019-04-24T22:44:39Z

src/nupic/regions/TMRegion.cpp

- "* 'cellsPerColumn'.",
+ "* 'cellsPerColumn' by default; if orColumnOutputs is "
+ "set, then this returns only numberOfCols. "
+ "The activations come from TM::getActiveCells(). ",


Good, I see you described what output 'bottomUpOut' contains. 👍

breznak · 2019-04-25T09:21:59Z

@ctrl-z-9000-times can you review as well when you've got time? You've requested changes, so I'll need your approval

breznak · 2019-04-25T09:23:36Z

It looks like the CI passed unit tests so I assume you figured out the problem with Windows. +1

unfortunately, I didn't. Just ifdefed Windows and delegated the fix to its dedicated issue #194

ctrl-z-9000-times

This looks good to me. I can't see why the test is failing on one platform.

breznak · 2019-04-25T12:10:12Z

Thanks for your reviews!

breznak added 9 commits April 11, 2019 09:39

TM::columnForCell update doc

4718b94

TMRegion: add check for orColumnsOutput

8403abc

that SDR.size matches TM.numberOfColumns

TM add method cellsToColumns()

cfdc9ba

which is a convenience wrapper for TM::columnForCell()

TMRegion: replace VectorHelpers::cellsToColumns with TM's version

b20c902

TM:cellsToColumns fix + unit test

19c3306

VectorHelpers: removed cellsToColumns and sparse variant,

4564a81

these methods have been replaced by TM.cellsToColumns. Because the conversion method is only valid for TM's cells.

Hotgym: example uses cellsToColumns

f0f559e

Hotgym: merge TM act + pred output to cols

340e4af

add check + print

TM: add getOutputColumns

b88b5f0

which represents TM's output as mini-columns, and is a union of active and predictive at the current time. Used by Hotgym example, and TMRegion

breznak added enhancement New feature or request ready TM code code enhancement, optimization, cleanup..programmer stuff labels Apr 11, 2019

breznak requested review from dkeeney and ctrl-z-9000-times April 11, 2019 17:06

breznak self-assigned this Apr 11, 2019

breznak commented Apr 11, 2019

View reviewed changes

fix OSX error

3a24836

ctrl-z-9000-times reviewed Apr 12, 2019

View reviewed changes

src/nupic/algorithms/TemporalMemory.cpp Show resolved Hide resolved

breznak added 2 commits April 15, 2019 17:14

Merge branch 'master_community' into columnForCell

cc8cd66

TM: removing getOutputColumns as not correct

bf4b96e

it does not make sense to mix feed-forward (active) activations with contextual (predictive)

breznak removed the ready label Apr 15, 2019

breznak mentioned this pull request Apr 15, 2019

TM anomaly #406

Merged

9 tasks

breznak added 2 commits April 17, 2019 07:46

Hotgym example: fix removed TM.getOutputColumns

e1d6ccc

TMRegion: rm option orColumnOutputs

ff036b8

as not supported, follows removal of TM::getColumnsOutput

breznak mentioned this pull request Apr 17, 2019

Random must not ignore seed in multiple instances #417

Closed

breznak added 5 commits April 17, 2019 16:53

Hotgym: split output for SP global/local

30d32a5

Hotgym: remove confused comment about Random

8e1ed03

there's no bug in Random, we're ok!

Hotgym: use function sin(x) as input

1c20daa

Hotgym: use RDSE encoder

4526a43

instead of Scalar

Hotgym: fix the example runtime

efaba05

use seed for RDSE, SDR use addNoise instead of randomize

ctrl-z-9000-times requested changes Apr 22, 2019

View reviewed changes

src/examples/hotgym/HelloSPTP.cpp Outdated Show resolved Hide resolved

src/examples/hotgym/HelloSPTP.cpp Outdated Show resolved Hide resolved

src/nupic/algorithms/TemporalMemory.cpp Outdated Show resolved Hide resolved

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

breznak added 3 commits April 23, 2019 11:44

Hotgym: split SP local,globa output

c8889e1

to be used for deterministic check and output

Merge branch 'master_community' into columnForCell

3eb4be4

review feedback

1121aaf

breznak dismissed dkeeney’s stale review via 1121aaf April 23, 2019 10:07

breznak added 4 commits April 23, 2019 13:50

fix err msg

5af9832

fix hotgym example

088d4a7

outSP SDR has to be initialized with dimensions, even if later successfully assigned (outSP = outSPglobal)

Merge branch 'master_community' into columnForCell

cb34890

Hotgym: bump CI time

d1b4395

for SP local, also remove the time check for Windows CI

breznak requested review from dkeeney and ctrl-z-9000-times April 23, 2019 12:40

fixing Win CI check

f43b9c0

WIP disable failing TM deterministic check

7dbf0e0

on Windows, fix later

ctrl-z-9000-times mentioned this pull request Apr 23, 2019

Deterministic Algorithms #194

Open

5 tasks

fix 2

9e5bffd

dkeeney approved these changes Apr 24, 2019

View reviewed changes

ctrl-z-9000-times approved these changes Apr 25, 2019

View reviewed changes

breznak merged commit b3243fc into master Apr 25, 2019

breznak deleted the columnForCell branch April 25, 2019 12:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

TM: cellsToColumns #397

TM: cellsToColumns #397

breznak commented Apr 11, 2019 •

edited

Loading

breznak Apr 11, 2019

ctrl-z-9000-times Apr 11, 2019

breznak Apr 12, 2019

ctrl-z-9000-times Apr 12, 2019

breznak Apr 12, 2019

breznak Apr 12, 2019

breznak Apr 15, 2019

dkeeney Apr 15, 2019

breznak Apr 15, 2019

breznak Apr 17, 2019

breznak Apr 11, 2019

ctrl-z-9000-times commented Apr 11, 2019

ctrl-z-9000-times commented Apr 11, 2019

breznak commented Apr 12, 2019 •

edited

Loading

ctrl-z-9000-times commented Apr 12, 2019

ctrl-z-9000-times commented Apr 12, 2019

ctrl-z-9000-times left a comment

breznak commented Apr 23, 2019

breznak commented Apr 23, 2019

dkeeney left a comment

dkeeney Apr 24, 2019

dkeeney Apr 24, 2019

breznak commented Apr 25, 2019

breznak commented Apr 25, 2019

ctrl-z-9000-times left a comment

breznak commented Apr 25, 2019

TM: cellsToColumns #397

TM: cellsToColumns #397

Conversation

breznak commented Apr 11, 2019 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ctrl-z-9000-times commented Apr 11, 2019

ctrl-z-9000-times commented Apr 11, 2019

breznak commented Apr 12, 2019 • edited Loading

ctrl-z-9000-times commented Apr 12, 2019

ctrl-z-9000-times commented Apr 12, 2019

ctrl-z-9000-times left a comment

Choose a reason for hiding this comment

breznak commented Apr 23, 2019

breznak commented Apr 23, 2019

dkeeney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

breznak commented Apr 25, 2019

breznak commented Apr 25, 2019

ctrl-z-9000-times left a comment

Choose a reason for hiding this comment

breznak commented Apr 25, 2019

breznak commented Apr 11, 2019 •

edited

Loading

breznak commented Apr 12, 2019 •

edited

Loading