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: add equals operator== #7

Closed
wants to merge 1 commit into from

Conversation

breznak
Copy link
Member

@breznak breznak commented Jan 19, 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

@breznak breznak added help wanted Extra attention is needed newbie labels Jan 19, 2018
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.

still work in progress;

  • need to decide how to implement the ==
  • some options for cleanup/simplifying the tests

src/nupic/algorithms/SpatialPooler.cpp Show resolved Hide resolved
sp.getBoostFactors(boostFactors2);
ASSERT_TRUE(check_vector_eq(boostFactors1, boostFactors2, numColumns));
delete[] boostFactors1;
delete[] boostFactors2;
Copy link
Member Author

Choose a reason for hiding this comment

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

This seems like some optimization. Q: keep the delete[] (saves memory), or remove it (saves cycles, as the local memory is unallocated anyway at the end of the function/scope)

sp2.getSynPermTrimThreshold()));
cout << "check: " << sp1.getSynPermActiveInc() << " " <<
sp2.getSynPermActiveInc() << endl;
ASSERT_TRUE(almost_eq(sp1.getSynPermActiveInc(),
Copy link
Member Author

Choose a reason for hiding this comment

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

TODO: many uses of almost_eq() can be replaced with gtest native EXCEPT_NEAR(a, b, diff); and the function can be removed from SPTest class.
https://github.com/google/googletest/blob/master/googletest/docs/AdvancedGuide.md

Although, for the comparisons we need a boolean, not an assert; so we'd have to keep the function?

We'll just implement the small almost_eq method ; see "twice" in the example
numenta#1384 (comment)

auto boostFactors2 = new Real[numColumns];
sp1.getBoostFactors(boostFactors1);
sp2.getBoostFactors(boostFactors2);
ASSERT_TRUE(check_vector_eq(boostFactors1, boostFactors2, numColumns));
Copy link
Member Author

Choose a reason for hiding this comment

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

Another function is check_vector_eq(); used in two scenarios:

  • comparing array[] with vector<>. this could be avoided ei by vector.data()
  • for 2 vectors of Real, it calls almost_eq element-wise; I'm not sure if that can be done implicitly without the need for such function?

check_vector_eq should be implemented as std::equal() with the "twice" example:
http://www.tenouk.com/cpluscodesnippet/cplusstlvectoralgorithmequal.html

auto potential2 = new UInt[numInputs];
sp1.getPotential(i, potential1);
sp2.getPotential(i, potential2);
ASSERT_TRUE(check_vector_eq(potential1, potential2, numInputs));
Copy link
Member Author

Choose a reason for hiding this comment

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

This is the case where check is within a for loop, so we'll have to create a variable that covers the whole loop:

bool allPotentialsEq = true;
for (..)
  allPotentialsEq == allPotEq && check_vector_eq(..)

@breznak breznak added this to the Community fixes milestone Jan 23, 2018
@breznak breznak removed this from the Community fixes milestone Aug 1, 2018
@breznak breznak added code code enhancement, optimization, cleanup..programmer stuff and removed feature new feature, extending API labels Aug 9, 2018
@dkeeney
Copy link

dkeeney commented Aug 19, 2018

don't know if this is the place to mention this or not.
As I am working on serialization I noticed some changes in the master related to the operator==. In particular the one for Region. I like the idea of having one for region but the region compare does not compare the RegionImpl object which basically holds the state for the region. The Region class is just a facade for the plugin (with base of RegionImpl) attached.

I think the only way to do this right is to add a virtual operator==() in RegionImpl such that it will force all implementations to implement it.

@breznak
Copy link
Member Author

breznak commented Aug 19, 2018

or remove it from the Region.

think the only way to do this right is to add a virtual operator==() in RegionImpl such that it will force all implementations to implement it.

but I think this sounds reasonable, a RegionImpl would compare some of its settings, and then defer to sp/tm/...==

@dkeeney
Copy link

dkeeney commented Aug 19, 2018 via email

@breznak breznak closed this Nov 28, 2018
@breznak breznak reopened this Nov 28, 2018
@breznak breznak self-assigned this Nov 29, 2018
@breznak breznak mentioned this pull request Nov 29, 2018
@breznak
Copy link
Member Author

breznak commented Nov 29, 2018

Replaced by #133

@breznak breznak closed this Nov 29, 2018
@breznak breznak deleted the sp_add_equals branch November 29, 2018 19:42
@breznak breznak mentioned this pull request Feb 26, 2019
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 help wanted Extra attention is needed in_progress newbie SP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants