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

SP equals #133

Merged
merged 2 commits into from
Nov 30, 2018
Merged

SP equals #133

merged 2 commits into from
Nov 30, 2018

Conversation

breznak
Copy link
Member

@breznak breznak commented Nov 29, 2018

Aims to implement equals for two SpatialPooler instances for more convenient comparisons & tests.

Fixes: numenta#1383
Used for numenta#1380
Moved from numenta#1384
replaces #7

@breznak breznak self-assigned this Nov 29, 2018
@breznak breznak added enhancement New feature or request SP code code enhancement, optimization, cleanup..programmer stuff labels Nov 29, 2018
@breznak breznak requested a review from dkeeney November 29, 2018 19:26
Copy link
Member Author

@breznak breznak left a comment

Choose a reason for hiding this comment

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

Please review

@@ -2020,6 +2020,8 @@ TEST(SpatialPoolerTest, testConstructorVsInitialize) {

// The two SP should be the same
check_spatial_eq(sp1, sp2);
EXPECT_EQ(sp1, sp2);
Copy link
Member Author

Choose a reason for hiding this comment

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

my question is, if we should delete the method check_spatial_eq() from SP tests. It is now obsoleted by SP.equals(), but this method is more verbose (asserts tell you where exactly is the diff. But, the comparison of SP.equals could do that too). On the other hand, the method is some burden to maintain (esp porting to other platforms etc)

Copy link

Choose a reason for hiding this comment

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

I know from experience that when debugging the SP comparison tests, having something that can tell you what it was that did not match is extremely helpful. There are so many things that must match...all affected by different parts of the code. Without some help it is hard to tell where to start looking.

On the other hand, it is indeed more to maintain. Tough call.

@breznak breznak added the ready label Nov 29, 2018
@breznak breznak mentioned this pull request Nov 29, 2018
Copy link

@dkeeney dkeeney left a comment

Choose a reason for hiding this comment

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

Looks good to me. It would also be ok with me if you want to have some way of being able to display what miss-matched when it fails,.

@breznak
Copy link
Member Author

breznak commented Nov 30, 2018

Thanks! I'll merge now and we can add the diff() function to SP later.

@breznak breznak merged commit 199df95 into master Nov 30, 2018
@breznak breznak deleted the sp_equals2 branch November 30, 2018 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
code code enhancement, optimization, cleanup..programmer stuff enhancement New feature or request ready SP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants