NUP-2368 and NUP-2328 Implement comprehensive Delayed Link test and fix delayed link data propagation issue #1264

vitaly-krugl · 2017-03-30T06:01:56Z

@scottpurdy @subutai @ywcui1990

UPDATE: I debugged and fixed the delayed link data propagation issue. The test L2L4WithDelayedLinksAndPhases verifies the fix using an L2/L4-like network with all phases enabled.

The fix updates data in all delayed link buffers at the end of every time step in Network::run(); it also implements direct copying of data from source to destination for 0-delay links in Link::compute().

L2L4WithDelayedLinksAndPhases:
I implemented the test L2L4WithDelayedLinksAndPhases that simulates the L2 and L4 layers (per our discussion on Monday) with R1/R2 in phase 1 and R3/R4 in phase 2; feed-forward links with delay=0 from R1/R2 to R3/R4, respectively; lateral links with delay=1 between R3 and R4; feedback links with delay=1 from R3/R4 to R1/R2, respectively.

Here is the input/output matrix for the new test L2L4WithDelayedLinksAndPhases, as initially implemented: L2L4 Test Case All Phases Running Inputs+Outputs.pdf

L2L4With1ColDelayedLinksAndPhase1OnOffOn:
Implemented additional unit test L2L4With1ColDelayedLinksAndPhase1OnOffOn with a single L4/L2 column that validates processing of incoming delayed and outgoing non-delayed link in the context of a region within a disabled phase.

  // This test simulates a network with a single L2/L4 column, structured as
  // follows:
  // o R1 ("L4") is in phase 1; R3 ("L2") is in phase 2;
  // o feed-forward link with delay=0 from R1 to R3
  // o lateral link with delay=1 looping back from R3 to itself
  // o feedback link with delay=1 from R3 to R1
  //
  // Running the network:
  // o Run 1 time step with both phases enabled
  // o Disable phase 1 and run two time steps
  // o Enable phase 1 and run two time steps

Here is the table of the expected inputs/outputs of the test's implementation as of commit e0bfeb9:
L2L4-1col-phase1OnOffOn.pdf

SingleL4RegionWithDelayedLoopbackInAndPhaseOnOffOn:
Implemented the final unit test SingleL4RegionWithDelayedLoopbackInAndPhaseOnOffOn that

  // Validates processing of outgoing/incoming delayed link in the context of a
  // region within a dispabled phase.
  //
  // This test simulates a network with a single L4 region, structured as
  // follows:
  // o R1 ("L4") is in phase 1
  // o feedback link with delay=1 from R1 to R1 (loopback)
  //
  // Running the network:
  // o Run 1 time step with phase 1 enabled
  // o Disable phase 1 and run two time steps
  // o Enable phase 1 and run two time steps

Here is the table of the expected inputs/outputs of the test's implementation as of commit c38b87c:
SingleL4WithDelayedLoopbackIn-phaseOnOffOn.pdf

…lasses and wired up the network. Getting runtime failures in L2L4WithDelayedLinksAndPhases test related to dimensions of linked regions.

vitaly-krugl · 2017-03-30T06:10:08Z

@subutai: I am misunderstanding something about region dimensions. I wired up the complete test network [R1 .. R4] as we discussed on Monday with you and Yuwei. See the new test block L2L4WithDelayedLinksAndPhases in this PR. I wanted the data for each region input and output in my test to be a buffer containing a single UInt64 value. I thought that to accomplish this, I would need to set dimensions on each region as 1x1, but that produced an exception advising me that dimensions must be multiples of two. Subsequently, I changed all the dimension to 2x2, but then net.initialize() failed as follows:

[ RUN      ] InputTest.L2L4WithDelayedLinksAndPhases
ERR:  Inconsistent dimension specification encountered. Region R1 has dimensions [2 2] but link [R3.out (region dims: [2 2])  to R1.feedbackIn (region dims: [2 2])  type: TestFanIn2] requires dimensions [1 1]. Additional information on region dimensions: Specified explicitly in setDimensions() [/Users/vkruglikov/nta/nupic.core/src/nupic/engine/Input.cpp line 479]
unknown file: Failure
C++ exception with description "Inconsistent dimension specification encountered. Region R1 has dimensions [2 2] but link [R3.out (region dims: [2 2])  to R1.feedbackIn (region dims: [2 2])  type: TestFanIn2] requires dimensions [1 1]. Additional information on region dimensions: Specified explicitly in setDimensions()" thrown in the test body.
[  FAILED  ] InputTest.L2L4WithDelayedLinksAndPhases (0 ms)

scottpurdy · 2017-03-30T16:51:10Z

@vitaly-krugl - Can you use single dimensional inputs/outputs instead of 2D? It could be some bad linking logic related to multi-dimensional links.

vitaly-krugl · 2017-03-30T17:11:09Z

@scottpurdy, regarding

Can you use single dimensional inputs/outputs instead of 2D? It could be some bad linking logic related to multi-dimensional links

Is the application of dimensions documented somewhere? I would like to understand it better. The other tests seem to be using two dimensions, so I followed suit.

By single-dimensional inputs/outputs, do you mean that I should do something like this?:

  Dimensions d1;
  d1.push_back(1);
  r1->setDimensions(d1);
  r2->setDimensions(d1);
  r3->setDimensions(d1);
  r4->setDimensions(d1);

scottpurdy · 2017-03-30T19:26:36Z

By single-dimensional inputs/outputs, do you mean that I should do something like this?

Yes. Does that resolve the issue?

Also, I would recommend using UniformLink rather than the others since that is pretty much all we usually use now and the others may have bugs.

vitaly-krugl · 2017-03-30T19:51:00Z

@scottpurdy, It didn't help. Got this error:

[ RUN      ] InputTest.L2L4WithDelayedLinksAndPhases
ERR:  Invalid source dimensions [1] for link [R3.out (region dims: [1])  to R1.feedbackIn (region dims: [1])  type: TestFanIn2]. Dimensions must be multiples of 2 [/Users/vkruglikov/nta/nupic.core/src/nupic/engine/TestFanIn2LinkPolicy.cpp line 61]
unknown file: Failure
C++ exception with description "Invalid source dimensions [1] for link [R3.out (region dims: [1])  to R1.feedbackIn (region dims: [1])  type: TestFanIn2]. Dimensions must be multiples of 2" thrown in the test body.

All tests in InputTest.cpp are using the TestFanIn2link policy for some reason. I will try with UniformLink.

vitaly-krugl · 2017-03-30T19:56:34Z

@scottpurdy - using the UniformLink policy allowed me to move beyond that issue. Thx.

…ailure in data propagation through the delayed feedback links.

… in the test.

…ssertion comments.

…ers of all links within a newtwork are refreshed at the end of each time step; and 0-delay links copy data directly from src to dest, bypassing the circular buffer altogether.

vitaly-krugl · 2017-04-01T17:24:56Z

@Thanh-Binh, you requested access to "L2/L4 Test Case All Phases Running Ins/Outs". I replaced that google docs URL with a PDF attachment in this github pull request, so that anyone can access it now without access permission. Hope this helps.:

L2L4 Test Case All Phases Running Inputs+Outputs.pdf

ywcui1990

@vitaly-krugl Done with my review. I left a few comments.

ywcui1990 · 2017-04-03T17:55:28Z

src/test/unit/engine/InputTest.cpp

+    ns->parameters.add(
+      "k",
+      ParameterSpec(
+        "Constant k valud for output computation", // description


Typo: "valud"

valud ==> value

thx

ywcui1990 · 2017-04-03T17:56:40Z

src/test/unit/engine/InputTest.cpp

+        ParameterSpec::ReadWriteAccess));
+
+    /* ----- inputs ------- */
+    ns->inputs.add(


It might be good add a feedforward input to L4 Regions to match the use case of L4-L2 network

@ywcui1990, do you believe that the feedforward inputs from R1/R2 to R3/R4 don't adequately test the feedforward links? I ask this, because we're trying to test links and integration of links and inputs/outputs/regions/network, and I think that the current test covers all those integration points.

OK, I agree with you on this.

ywcui1990 · 2017-04-03T18:00:01Z

src/test/unit/engine/InputTest.cpp

+
+    baseOutputBuffer[0] = ffInput[0] + latInput[0];
+    baseOutputBuffer[1] = ffInput[0];
+    baseOutputBuffer[2] = latInput[0];


I understand we need to keep track of ffInput and latInput for test purpose. However, I find it confusing to use baseOutputBuffer[1:2] to track them, since these are not outputs of the region. Should we create a separate baseInputBuffer instead?

@ywcui1990, I wanted to make the test logic less error-prone. Keeping it all together prevents out-of-sync/off-by-one type errors. For consistency, in these test-only regions, the result of the computation is always at index 0, and the other values are inputs that went into that computation. I document this both in each of the region classes as well as to the right of every ASSERT statement to make it easy to follow.

Perhaps we could improve the comments to make it easier to follow. Would you like to suggest some? Thank you.

Adding some comments would help. How about something like
"Only the first element of baseOutputBuffer represents region output. We keep track of inputs to the region using the rest of the baseOutputBuffer vector. These inputs are used in the tests"

…th a single L4/L2 column that validates processing of incoming delayed and outgoing non-delayed link in the context of a region within a suppressed phase.

vitaly-krugl · 2017-04-03T20:13:13Z

Implemented additional unit test L2L4With1ColDelayedLinksAndPhase1OnOffOn with a single L4/L2 column that validates processing of incoming delayed and outgoing non-delayed link in the context of a region within a disabled phase.

  // This test simulates a network with a single L2/L4 column, structured as
  // follows:
  // o R1 ("L4") is in phase 1; R3 ("L2") is in phase 2;
  // o feed-forward link with delay=0 from R1 to R3
  // o lateral link with delay=1 looping back from R3 to itself
  // o feedback link with delay=1 from R3 to R1
  //
  // Running the network:
  // o Run 1 time step with both phases enabled
  // o Disable phase 1 and run two time steps
  // o Enable phase 1 and run two time steps

Here is the table of the expected inputs/outputs of the test's implementation as of commit e0bfeb9:
L2L4-1col-phase1OnOffOn.pdf

…aseOnOffOn that validates processing of outgoing/incoming delayed link in the context of a region within a dispabled phase.

vitaly-krugl · 2017-04-03T21:07:14Z

Implemented the final unit test SingleL4RegionWithDelayedLoopbackInAndPhaseOnOffOn that

  // Validates processing of outgoing/incoming delayed link in the context of a
  // region within a dispabled phase.
  //
  // This test simulates a network with a single L4 region, structured as
  // follows:
  // o R1 ("L4") is in phase 1
  // o feedback link with delay=1 from R1 to R1 (loopback)
  //
  // Running the network:
  // o Run 1 time step with phase 1 enabled
  // o Disable phase 1 and run two time steps
  // o Enable phase 1 and run two time steps

Here is the table of the expected inputs/outputs of the test's implementation as of commit c38b87c:
SingleL4WithDelayedLoopbackIn-phaseOnOffOn.pdf

…nts gated by the value of macro _LINK_DEBUG. This preserves functionality that I found very useful for debugging data flow through the links.

vitaly-krugl · 2017-04-03T21:33:58Z

src/nupic/engine/Link.cpp

+
+// Set this to true when debugging to enable handy debug-level logging of data
+// moving through the links, including the delayed link transitions.
+#define _LINK_DEBUG false


I discussed this with @scottpurdy - he approved.

vitaly-krugl · 2017-04-03T21:40:25Z

cc @ywcui1990: the two additional tests L2L4With1ColDelayedLinksAndPhase1OnOffOn and SingleL4RegionWithDelayedLoopbackInAndPhaseOnOffOn are ready for your review.

cc @scottpurdy This is ready for full code review. @ywcui1990 is only reviewing the unit tests, so we need someone to review the fix.

vitaly-krugl · 2017-04-03T22:01:52Z

This is ready and begging for complete code review :)

scottpurdy

There are three TODOs listed in the PR description - what are the statuses of those?

I reviewed all of the code except the test file so far and it all looks good aside from my recommendation to remove the ArrayUtils.hpp file.

scottpurdy · 2017-04-03T23:58:54Z

src/nupic/utils/ArrayUtils.hpp

+      auto srcData = (SourceElementT*)inbuf;
+      std::stringstream ss;
+
+      for (size_t i=0; i < numElements; ++i)


Personally, I think it would be better to just put this simple for loop as the implementation for Array::operator<<. Maybe you still need a wrapper around it but overall it would be less code and I don't think it makes sense to try to create reusable functions for such a simple task. Plus, the potential use cases won't always want ", " as the delimiter.

If you really wanted it to be reusable then I think something like a combination of std::transform to convert to strings and boost::algorithm::join to join using some delimiter would be the most generic but I think has many more string allocations than you'd like.

vitaly-krugl · 2017-04-04T04:51:40Z

@scottpurdy, regarding

There are three TODOs listed in the PR description - what are the statuses of those?

They were all completed. I removed them from the PR description just now. Thanks.

ywcui1990

The tests look good to me. I left a few minor comments.

ywcui1990 · 2017-04-04T17:07:58Z

src/test/unit/engine/InputTest.cpp

+TEST(InputTest, SingleL4RegionWithDelayedLoopbackInAndPhaseOnOffOn)
+{
+  // Validates processing of outgoing/incoming delayed link in the context of a
+  // region within a dispabled phase.


typo: "dispabled"

ywcui1990 · 2017-04-04T17:08:42Z

src/test/unit/engine/InputTest.cpp

+  // This test simulates a network with a single L4 region, structured as
+  // follows:
+  // o R1 ("L4") is in phase 1
+  // o feedback link with delay=1 from R1 to R1 (loopback)


It's better to call it lateral link rather than feedback link since there is only one region.

vitaly-krugl · 2017-04-04T18:08:20Z

Thank's @ywcui1990, I addressed your feedback via 8a88a0f.

…rrayBase.

vitaly-krugl · 2017-04-04T20:50:49Z

@scottpurdy, I implemented operator<< in ArrayBase, so it's now cleaner to use nupic Arrays in logging/streaming statements. The operator still makes use of the ArrayUtils::streamBufferOfBasicType utility function in ArrayUtils.hpp, which may also be used to dump C-style arrays of NTA type elements. See 8aad13c

…Utils.hpp.

vitaly-krugl · 2017-04-05T01:20:04Z

@scottpurdy, I got rid of the new file ArrayUtils.hpp and relocated the array-printing logic to ArrayBase. Let's discuss any further modifications on Wednesday.

NUP-2368 Updated description of Link::shiftBufferedData as recommended by Scott. NUP-2368 Clarified intended propagation of data depending on the propagationDelay arg in the Link constructor documentation.

vitaly-krugl · 2017-04-05T18:44:12Z

@scottpurdy, I made the final changes that we discussed this morning. See 612c5c8

NUP-2368 Work-in-progress Implemented L2TestRegion and L4TestRegion c…

3c8b22d

…lasses and wired up the network. Getting runtime failures in L2L4WithDelayedLinksAndPhases test related to dimensions of linked regions.

vitaly-krugl changed the title ~~WORK IN PROGRESS NUP-2368 Test delayed link~~ WORK IN PROGRESS NUP-2368 Implement comprehensive Delayed Link test Mar 30, 2017

Vitaly Kruglikov added 4 commits March 30, 2017 18:43

NUP-2368 Completed the test implementation. The test demonstrates a f…

10e34f4

…ailure in data propagation through the delayed feedback links.

NUP-2368 Added comments to help readers better understand the asserts…

cf365c3

… in the test.

NUP-2368 Fix R4 feedForwardIn assert in iteration 3; improve output a…

c19157d

…ssertion comments.

NUP-2368 Fixed servicing of delayed links such that the curcular buff…

694ff49

…ers of all links within a newtwork are refreshed at the end of each time step; and 0-delay links copy data directly from src to dest, bypassing the circular buffer altogether.

vitaly-krugl changed the title ~~WORK IN PROGRESS NUP-2368 Implement comprehensive Delayed Link test~~ WORK IN PROGRESS NUP-2368 NUP-2328 Implement comprehensive Delayed Link test and fix delayed link data propagation issue Apr 1, 2017

This was referenced Apr 1, 2017

NUP-2389 Remove calls to Region::purgeInputLinkBufferHeads numenta/nupic-legacy#3508

Merged

Update nupic not to call purgeInputLinkBufferHeads after nupic.core changes are deployed numenta/nupic-legacy#3509

Closed

ywcui1990 reviewed Apr 3, 2017

View reviewed changes

NUP-2368 Implemented test L2L4With1ColDelayedLinksAndPhase1OnOffOn wi…

e0bfeb9

…th a single L4/L2 column that validates processing of incoming delayed and outgoing non-delayed link in the context of a region within a suppressed phase.

NUP-2368 Implement unit test SingleL4RegionWithDelayedLoopbackInAndPh…

c38b87c

…aseOnOffOn that validates processing of outgoing/incoming delayed link in the context of a region within a dispabled phase.

NUP-2368 Wrap NTA_DEBUG statements in Link.cpp in conditional stateme…

f42b8ae

…nts gated by the value of macro _LINK_DEBUG. This preserves functionality that I found very useful for debugging data flow through the links.

vitaly-krugl commented Apr 3, 2017

View reviewed changes

vitaly-krugl changed the title ~~WORK IN PROGRESS NUP-2368 NUP-2328 Implement comprehensive Delayed Link test and fix delayed link data propagation issue~~ NUP-2368 and NUP-2328 Implement comprehensive Delayed Link test and fix delayed link data propagation issue Apr 3, 2017

scottpurdy reviewed Apr 4, 2017

View reviewed changes

ywcui1990 reviewed Apr 4, 2017

View reviewed changes

NUP-2368 Addressed Yuwei's code review feedback.

8a88a0f

NUP-2368 Address code review feedback: create operator<< for nupic::A…

8aad13c

…rrayBase.

Vitaly Kruglikov added 6 commits April 4, 2017 13:55

Added missing trailing newline

fd40439

Removed accidentally-pasted region initialization loop from Network::run

707d6d0

Clean up ArrayUtils::_templatedStreamBuffer a little.

1be641b

add const to the end var

25f70a0

Remove unnecessary whitespace in switch statement.

7aa939e

NUP-2368 Moved array printing logic to ArrayBase. Removed utils/Array…

eefc59e

…Utils.hpp.

Vitaly Kruglikov added 3 commits April 4, 2017 18:31

Guard against empty array case in ArrayBase::_templatedStreamBuffer

759857f

NUP-2368 Moved Link unit tests to a source dedicated file LinkTest.cpp.

612c5c8

NUP-2368 Updated description of Link::shiftBufferedData as recommended by Scott. NUP-2368 Clarified intended propagation of data depending on the propagationDelay arg in the Link constructor documentation.

Remove unnecessary newline at end of LinkTest.cpp

25cd1a4

scottpurdy approved these changes Apr 5, 2017

View reviewed changes

vitaly-krugl merged commit 5dfb6af into numenta:master Apr 5, 2017

vitaly-krugl deleted the nup2368-unit-test-delayed-link-fix branch April 5, 2017 20:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

NUP-2368 and NUP-2328 Implement comprehensive Delayed Link test and fix delayed link data propagation issue #1264

NUP-2368 and NUP-2328 Implement comprehensive Delayed Link test and fix delayed link data propagation issue #1264

vitaly-krugl commented Mar 30, 2017 •

edited

vitaly-krugl commented Mar 30, 2017 •

edited

scottpurdy commented Mar 30, 2017

vitaly-krugl commented Mar 30, 2017

scottpurdy commented Mar 30, 2017

vitaly-krugl commented Mar 30, 2017

vitaly-krugl commented Mar 30, 2017

vitaly-krugl commented Apr 1, 2017 •

edited

ywcui1990 left a comment

ywcui1990 Apr 3, 2017

vitaly-krugl Apr 3, 2017

ywcui1990 Apr 3, 2017

vitaly-krugl Apr 3, 2017 •

edited

ywcui1990 Apr 4, 2017

ywcui1990 Apr 3, 2017

vitaly-krugl Apr 3, 2017

ywcui1990 Apr 4, 2017

vitaly-krugl commented Apr 3, 2017 •

edited

vitaly-krugl commented Apr 3, 2017

vitaly-krugl Apr 3, 2017

vitaly-krugl commented Apr 3, 2017

vitaly-krugl commented Apr 3, 2017 •

edited

scottpurdy left a comment

scottpurdy Apr 3, 2017

vitaly-krugl commented Apr 4, 2017

ywcui1990 left a comment

ywcui1990 Apr 4, 2017

ywcui1990 Apr 4, 2017

vitaly-krugl commented Apr 4, 2017

vitaly-krugl commented Apr 4, 2017 •

edited

vitaly-krugl commented Apr 5, 2017

vitaly-krugl commented Apr 5, 2017

NUP-2368 and NUP-2328 Implement comprehensive Delayed Link test and fix delayed link data propagation issue #1264

NUP-2368 and NUP-2328 Implement comprehensive Delayed Link test and fix delayed link data propagation issue #1264

Conversation

vitaly-krugl commented Mar 30, 2017 • edited

vitaly-krugl commented Mar 30, 2017 • edited

scottpurdy commented Mar 30, 2017

vitaly-krugl commented Mar 30, 2017

scottpurdy commented Mar 30, 2017

vitaly-krugl commented Mar 30, 2017

vitaly-krugl commented Mar 30, 2017

vitaly-krugl commented Apr 1, 2017 • edited

ywcui1990 left a 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

vitaly-krugl Apr 3, 2017 • edited

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

vitaly-krugl commented Apr 3, 2017 • edited

vitaly-krugl commented Apr 3, 2017

Choose a reason for hiding this comment

vitaly-krugl commented Apr 3, 2017

vitaly-krugl commented Apr 3, 2017 • edited

scottpurdy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitaly-krugl commented Apr 4, 2017

ywcui1990 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vitaly-krugl commented Apr 4, 2017

vitaly-krugl commented Apr 4, 2017 • edited

vitaly-krugl commented Apr 5, 2017

vitaly-krugl commented Apr 5, 2017

vitaly-krugl commented Mar 30, 2017 •

edited

vitaly-krugl commented Mar 30, 2017 •

edited

vitaly-krugl commented Apr 1, 2017 •

edited

vitaly-krugl Apr 3, 2017 •

edited

vitaly-krugl commented Apr 3, 2017 •

edited

vitaly-krugl commented Apr 3, 2017 •

edited

vitaly-krugl commented Apr 4, 2017 •

edited