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

Performance benchmark for Connections #490

Merged
merged 30 commits into from
Jul 27, 2015

Conversation

chetan51
Copy link
Contributor

@chetan51 chetan51 commented Jul 9, 2015

Fixes #491.

Usage: make tests_connections_performance

@chetan51
Copy link
Contributor Author

chetan51 commented Jul 9, 2015

@subutai @scottpurdy

@chetan51 chetan51 changed the title Initial test harness for Connections benchmark Placeholder: Initial test harness for Connections benchmark Jul 9, 2015
@subutai
Copy link
Member

subutai commented Jul 9, 2015

@chetan51 Great to see some initial progress. One thought on this: what if we trained general temporal memory on the full hotgym dataset (or ideally an even longer dataset) and saved the connections datastructure. One of the benchmarks could simply load it and then use the various API calls on that structure in a long loop. ??

@chetan51
Copy link
Contributor Author

chetan51 commented Jul 9, 2015

@subutai That's a good idea for one of the tests. We would also want to exercise the actual process of learning from scratch as well, maybe in separate tests.

@chetan51
Copy link
Contributor Author

chetan51 commented Jul 9, 2015

Also we might be able to use numenta/nupic-legacy#2066 to do the same except with the Spatial Pooler.

# Setup test_connections_performance
#
set(EXECUTABLE_CONNECTIONSPERFORMANCETEST connections_performance_test)
add_executable(${EXECUTABLE_CONNECTIONSPERFORMANCETEST} test/integration/ConnectionsPerformanceTest.cpp)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Break this line

@scottpurdy
Copy link
Contributor

If the tests are all in one file then no need for the header. How big is the resulting binary? It should be pretty small.

@chetan51
Copy link
Contributor Author

@scottpurdy It's 552K. I kept a separate header to keep consistent with the other tests in nupic.core. Do you want me to remove it?

@scottpurdy
Copy link
Contributor

@chetan51 - yeah I would say to remove it. This isn't a normal test, it is a stand-alone executable. Plus, our tests should get moved over to using the gtest framework in which you don't have headers for the tests.

Edit: Actually it might be good to keep the header in case someone else wants to create an executable wrapping these tests.

@scottpurdy
Copy link
Contributor

👍

@chetan51 chetan51 changed the title Placeholder: Initial test harness for Connections benchmark Initial test harness for Connections benchmark Jul 17, 2015
@chetan51
Copy link
Contributor Author

@scottpurdy Implemented the spatial pooler test, still have to implement the temporal pooler test. Please review.

virtual void RunTests();

private:
void testTemporalMemoryUsage();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of having a header is so that someone can create their own program that wraps this. So I would make the test methods public in case the caller doesn't want to run all of them, which is likely the case when profiling.

@chetan51
Copy link
Contributor Author

Added Temporal Pooler test, and modified SP test. Unfortunately the TP test is segfaulting for some reason, so I'll need to debug that now.

@chetan51
Copy link
Contributor Author

@scottpurdy Fixed the TP test. The following 4 tests are implemented: spatial pooler, temporal memory, large temporal memory, temporal pooler. This completes the benchmark.

This PR is ready for review now.

@scottpurdy
Copy link
Contributor

Can you move the different test cases to different PRs? Just easier to review.

@chetan51 chetan51 changed the title Initial test harness for Connections benchmark Performance benchmark for Connections Jul 22, 2015
@scottpurdy
Copy link
Contributor

One question about headers still outstanding and a few lines over 80 characters but otherwise looks good

@chetan51
Copy link
Contributor Author

@scottpurdy Fixed lines over 80 characters. Ready to merge?

@scottpurdy
Copy link
Contributor

@chetan51
Copy link
Contributor Author

@scottpurdy Done.

@scottpurdy
Copy link
Contributor

You need to add the Connections #include to the .cpp file. Right now you happen to be getting it from the TM include in the header file. But that include should also be moved to the .cpp file. Your header shouldn't need anything but forward declarations, which you haven't added yet. And the .cpp should have includes for anything it is using. We can sync quickly on this in the morning if you want.

Additionally, don't use the using namespace statement in header files if you can avoid it (which you can in this case) since it brings a bunch of things into scope in any .cpp file that include that header.

@chetan51
Copy link
Contributor Author

@scottpurdy Is that better?

@chetan51
Copy link
Contributor Author

@scottpurdy Updated.

@scottpurdy
Copy link
Contributor

👍 but did this cause the AppVeyor failure?

@chetan51
Copy link
Contributor Author

I can't tell why the AppVeyor build failed. @rhyolight can you tell?

@rhyolight
Copy link
Member

@chetan51 @rcrowder Go team! 🍏

chetan51 added a commit that referenced this pull request Jul 27, 2015
Performance benchmark for Connections
@chetan51 chetan51 merged commit eb0b420 into numenta:master Jul 27, 2015
@chetan51 chetan51 deleted the connections_benchmark branch July 27, 2015 18:21
@rcrowder
Copy link
Contributor

Excellent. I'm looking at the divergence in the extensive tests, but had some RL distractions to deal with..

breznak pushed a commit to breznak/nupic.core that referenced this pull request Jul 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants