Port python TM to C++, using C++ Connections data structure

[rhyolight]

The python temporal memory implementation is located here, and a subclass of it that uses the C++ Connections data structure is located here. Unit tests are located here.

This task is to port the above implementation to C++, so that we have a pure C++ version that uses the Connections data structure.

---

• [x] Create logical C++ API
• [x] Map C++ API to Python API in SWiG
• [x] Port existing Python unit tests to C++
• [x] Ensure random number generation is consistent between Python / C++
• [x] Implement Capnp serialization configuration for TM
• [x] Run exact same python test code run against C++ TM algorithm
• [x] Investigate optimizations on C++ TM code
• [x] Add TM constructor parameter checks
• [x] TM unit test testWrite function completion

[rcrowder]

@chetan51 Just to clarify. This would be the addition of the following new C++ files (plus associated .hpp headers);

• nupic/algorithms/TemporalMemory.cpp (based off of the temporal_memory.py)
• nupic/algorithms/FastTemporalMemory.cpp (based off of the fast_temporal_memory.py)
• test/unit/algorithms/TemporalMemoryTest.cpp
• test/unit/algorithms/FastTemporalMemoryTest.cpp

1 Would a TemporalMemoryProto.capnp be created as another PR, or expected as part of this port? 2 Is the temporal_memory_shim.py only need for Python? 3 Similar with the abstract_temporal_memory_test.py, only Python?

[chetan51]

@rcrowder

I don't think we need separate C++ versions of TemporalMemory and FastTemporalMemory, just one version that uses the C++ Connections data structure (analogous to fast_temporal_memory.py).

1 Would a TemporalMemoryProto.capnp be created as another PR, or expected as part of this port?

This could be a separate PR.

2 Is the temporal_memory_shim.py only need for Python?

Yes.

3 Similar with the abstract_temporal_memory_test.py, only Python?

Yes, it might be helpful to structure the test classes the same way in C++.

[rhyolight]

@rcrowder So are you thinking about tackling this port? :heart_eyes:

[rcrowder]

I've already started, branch ready :smile: Chetan has done a fantastic job with commenting the Python. My priority is still to see NuPIC working on Windows. But thought I'd chip away at this issue. Another way of learning the code, and particularly the new TM.

[rhyolight]

Outstanding!

[rcrowder]

@chetan51 Thanks. I'd made a start https://github.com/rcrowder/nupic.core/commit/94e229c37b3f02e460512eb4a5276c5158da8abf Lots left to do before it all hits the compiler :smile:

[rcrowder]

@chetan51 A quick question; It looks like there are optimizations in the Python Connection class (e.g. _synapsesForPresynapticCell mapping dictionary). Do you want me to port them as part of this PR? Or only use the current C++ class in it's present state, and replace any missing functionality with appropriate calls?

[chetan51]

@rcrowder You can safely ignore the Python Connections class, and use the C++ Connections class in its present state.

[rcrowder]

@chetan51 I'm just starting to compare py and cpp. Is this a valid alternative for determining winning cell? https://github.com/rcrowder/nupic.core/blob/314-Port-python-TM/src/nupic/algorithms/TemporalMemory.cpp#L343

[rcrowder]

Or alternatively, restart by adopting the Java port and using Python as a reference?

[chetan51]

@rcrowder In that line of code, you are looking for the presynaptic cell in the winner cells. But the temporal memory implementation looks for the segment's cell (the one the segment belongs to) among the winner cells. So I don't think that code will work correctly.

[rcrowder]

@chetan51 Thanks, I've progressed further. My latest commits to the branch [1] now includes all the unit tests ported and passing :smile: I needed to single step debug through all the tests code, and side-by-side compare with the Python, to make sure that everything was an exact match. Green lit Travis and AppVeyor build histories [2][3] Next up, saving & loading, and porting the integration tests. Integration test porting could be split off into another PR. It'd be great if you could have a look and maybe an early critique?

1 https://github.com/numenta/nupic.core/compare/master...rcrowder:314-Port-python-TM 2 https://travis-ci.org/rcrowder/nupic.core/builds 3 https://ci.appveyor.com/project/rcrowder/nupic-core-q6voj

[chetan51]

@rcrowder Great! I don't have much time to do a review right now, but I made a very cursory glance over the changes and it looks good overall. One question I have is, what's the difference between TemporalMemory.cpp and FastTemporalMemory.cpp? Why are both needed in C++?

[rcrowder]

@chetan51 Thanks. I wasn't sure myself of the need of the fast TM code. Should it be kept and moved, into the TM cpp, removed, or the changes adopted in the main TM class? No rush, I'm just working out what extra is required for the integration tests.

[chetan51]

The fast TM code in Python is a version that uses the C++ Connections data structure. Since the default C++ TM implementation should use the C++ Connections data structure, there is no need for an additional fast TM C++ implementation.

[rcrowder]

Super. I'll remove the cpp, and concentrate on the other tests. Cheers.

[chetan51]

Hi @rcrowder, just following up on this issue. How's it going, and are you blocked on anything?

[rcrowder]

Hey @chetan51 I think it's going ok. Work for me has been busy than normal over the last few weeks, but.. As of today, I've ported the majority of the Python code to C++ The surprised algorithm review the other week, occurred at an opportune time. I've been addressing those issues and continuing the port.

I'm now at the nice stage of single-step debugging through all the new unit tests and support classes (Trace, Metric, MonitorMixin, PatternMachine, and SequenceMachine). I noticed too late that the SP already has a Range function. Plus as I step through, I can compare to the original code and flesh out functions further (e.g. the init() calls in the TM Extensive Test). Eventually firing up a Linux VM to squash out GCC warnings before hitting Travis again.

Here's the comparison with core master - https://github.com/numenta/nupic.core/compare/master...rcrowder:314-Port-python-TM

I'd be great if you could have a quick review. Mainly to see that I'm on the right track, particularly algorithm conversion. Be aware that most of the code, and particularly data containers and types are W.I.P. It'd be great to hear anything that could be done differently. Or anything that is wrong with the port, for example the changes in Connections class.

[chetan51]

@rcrowder Taking a look at the comparison, I noticed that you are porting over all the files related to testing and monitoring the TM as well. This is not necessary, since we can use the Python tests and monitor mixins with the C++ TM as well, since it will have the same interface as the Python TM.

In the first PR for this port, it should be sufficient to have an implementation of TemporalMemory.hpp, TemporalMemory.cpp, and tests/unit/TemporalMemoryTest.cpp. In later PRs we can test the C++ TM with the existing Python TM tests.

Hope this doesn't mean you have to throw away a lot of the work you did. If so, I apologize for the lack of clarity.

Thanks!

[rhyolight]

I was supposed to break this issue out into subtasks three weeks ago based on a NDPR meeting. I'm sorry it's my fault this wasn't defined better. I'll try to do this today.

[rcrowder]

@chetan51 CC @scottpurdy

Thank you for the brief review. Porting the Monitor Mixin has added delay, so more than happy to push that work to another PR. It's not been wasted effort, it's certainly helped exposed tricky porting aspects (such as determining data container types). I was looking at using a STL Set for Cells, but too much work to refactor everywhere.

I've kept my original porting branch, but created a new clean one with just the TM and it's unit test. Hopefully the change in Connections.hpp is ok? Here's the PR with new branch linked to it; https://github.com/numenta/nupic.core/pull/409

@rhyolight You've have a lot on, and ideally I should have defined the tasks for you to add. I'm sorry. I hope to keep all the work that has been ported so far and continue to complete that (at a lower priority). It feels close to being a complete port. And at some point I'd like to address the following;

• SP::range_ function moved to a util class - Ready in branch (with unit tests)
• Add Pattern and Sequence machines - Ready in branch (with unit tests)
• Complete the port of MonitorMixin (with Trace and Metric) - Container issues
• Complete the port of TM unit tests (tutorial, extensive, and performance tests) - Partial port

Now to deal with the GCC issues on Travis, make sure interfaces are correct, and check all aspects from the algorithm review are dealt with...

[rhyolight]

@rcrowder I can help you debug issues on Ubuntu and OSX if you need it.

[rcrowder]

@rhyolight Thanks, got GCC working, just CLang issues now..

[chetan51]

@rcrowder Thanks for the PR, I'll take a look.

To be clear, we don't need to port the MonitorMixin stuff, or the extensive/performance TM tests (only need to port the unit tests). These other things can be run in Python, using SWIG bindings to test the C++ code (as long as the API is consistent between Python and C++).

[rcrowder]

@rhyolight Thanks for breaking down the sub-tasks. Does this mean that further commits still go to the branch and this PR, with the nod to reviewers on each sub-task when it is deemed ready?

[rhyolight]

@rcrowder Well, ideally each new issue would be attacked in the proper order, and each would have a PR associated with it that builds upon older PRs for older issues that have already been merged (or sometimes not merged yet). This can be a bit tricky to do, but it makes the PR reviews and approvals much easier and faster. Smaller PRs are almost always better if they are possible. Let me know what you think you can do. I'm sure we can reach a compromise.

[rhyolight]

I am re-opening this until I can verify that the subtasks in the description are truly fixed. CC @rcrowder @chetan51

[rhyolight]

@rcrowder I've lost track of what is happening to support this. If you are willing and able to join the NDPR today, can you talk a bit about it? If not, please respond here. Thanks! :rocket: :guitar:

[rcrowder]

cc @scottpurdy @chetan51

@rhyolight Sorry for missing the NDPR. I finally finished the 2 day move about 10:30, but was too tired for the 11pm meeting. I watched it this morning. Of the three remaining un-ticked tasks (out of order) -

1 The NuPIC SWIG binding work needs moving over to nupic.core The changes made look quite trivial, but getting approval of the interface could take longer. I don't like the different handling of containers between the TM and SP. Will move the binding work across from NuPIC, then seek Py advice from Scott and/or Chetan.

3 Optimization has been discussed and thought to be less important at the moment. Will add more info to that issue if needed.

2 This is the main stumbling block. Four integration tests, that have similar structure, all produce different stats between Py and C++ runs. They diverge after a few hundred cell segments, and associated synapses, are evolved. Which has made it tricky to narrow down and debug. The last thing I was looking at was Random::shuffle, and was at a point of replacing all with "MT_Rand" code to try and match NumPy random shuffle. Then Tom's change for Random overflow appeared. I need to update with that change to Random and see how it affects the stats. At the moment all points to shuffle being inconsistent over long runs between Py (NumPY) and C++ (Random.cpp). Then job hunting, and then Windows work made this slip further.

Now relocated, I can get this PR updated with the last months changes. And see how those integration tests fair, particularly with Tom's change. I hope from your end someone has been trying this out? And like David Ray, I need to check for any changes over the last month. Orphan decay made it in, other recent changes need porting. Whether they add to this PR, or as a separate issue, I'm not sure.

[scottpurdy]

@rcrowder - We should only be using the NuPIC Random class for RNG. If something else is used then in some places then it should be filed as a bug.

[rcrowder]

@scottpurdy Just had a closer look/refresh. The Py and C++ TM both do use Random, issue is with the NumPy random used in the pattern_machine.py Should this generator be using NuPIC and match the C++ port? Or adopt numpy random in the C++ pattern machine?

[chetan51]

@rcrowder Everything should use the NuPIC Random class, including pattern_machine.py.

[rcrowder]

@chetan51 Thanks for the update. I'm in the middle of fixing that up at the moment, and trying the TM C++ extensive tests with all the last few weeks changes.

[rcrowder]

@chetan51 FYI https://travis-ci.org/rcrowder/nupic/builds/79738939 contains the Python changes in Pattern and Sequence machines to use nupic::Random. Odd error relating to floats that I need to look at today/tomorrow.

[scottpurdy]

Is this blocked by #845 or #851 as well?

[chetan51]

Yes, I guess so.

[rcrowder]

Yes, I believe so too. Relates to https://github.com/numenta/nupic.core/issues/419 As I discovered with the higher level sequence testing of the C++ TM, the C++ side was constructing more synapses as Sagan saw recently (in the hundreds extra range, in comparison to the thousands created in the tests). It certainly looked related to unknown issues in the Connections class. Some marginally error appearing, that may have been solved with Chetan's epilson checking commit recently (https://github.com/numenta/nupic.core/pull/846).

[rcrowder]

@chetan51 From the looks of recent PRs, have you been able to resolve this issue? It'd be great to clean this out, before the suggested move to second optimization stage occurs (at Numenta), i.e. working out faster ways to get variable-sized data from Python into the C++ TM. Which I believe maybe a big performance bottleneck. Something I struggled to work out cleanly last year, and highlighted in a conference call with Scott and Subutai last summer as this work progressed remotely before other priorities stepped forward.

[rhyolight]

@rcrowder You should know that @chetan51 has left Numenta to work on independent mobile game development. I'm not sure if he will continue to be active on our projects or not. So tagging @scottpurdy and @subutai to continue this discussion.

[scottpurdy]

@rcrowder - The C++ TM should be fully usable now. The compatibility tests show some differences still but we aren't going to invest in resolving those right now.

[mrcslws]

This has been finished for a while. (The compatibility tests pass.)