WIP SP: add equals operator== #1384
Conversation
| const UInt numInputs = this->getNumInputs(); | ||
|
|
||
| ASSERT_TRUE(this->getNumColumns() == sp.getNumColumns()); | ||
| ASSERT_TRUE(this->getNumInputs() == sp.getNumInputs()); |
There was a problem hiding this comment.
RFC: I'm looking for a way how to approach implementation of these comparisons.
The method in SPTest class used man ASSERT_TRUE() calls, the advantage is that when there is an inequality you'll know what exactly caused it.
Now for equals== implementation we need to turn the asserts into a conjunction of conditions (a1==b1 && a2==b2 &&...).
I was thinking of turning this
ASSERT_TRUE(this->getNumColumns() == sp.getNumColumns());
ASSERT_TRUE(this->getNumInputs() == sp.getNumInputs());
Into
- A)
(a1==b1) || return false; but this result in losing the information on which specific check the comparison failed. Is that a problem? - B) If the above is a problem, we could return a huge conjunction
return (this->getNumColumns() == sp.getNumColumns()) && (this->getNumInputs()==sp.getNumInputs()) && ..
I think the b) would inform us in gtest at which part the conjunction failed, keeping the desired behavior.
The only problem with this is the readability and that some checks have pre-processing or even are inside for-loops, but we could workaround that.
- C) keep the asserts (
EXPECT_EQ) for information and just add && for equals.
EDIT:
add option C)
| sp.getBoostFactors(boostFactors2); | ||
| ASSERT_TRUE(check_vector_eq(boostFactors1, boostFactors2, numColumns)); | ||
| delete[] boostFactors1; | ||
| delete[] boostFactors2; |
There was a problem hiding this comment.
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.getGlobalInhibition()); | ||
| ASSERT_TRUE(sp1.getNumActiveColumnsPerInhArea() == | ||
| sp2.getNumActiveColumnsPerInhArea()); | ||
| ASSERT_TRUE(almost_eq(sp1.getLocalAreaDensity(), |
There was a problem hiding this comment.
TODO: many uses of almost_eq() can be replaced with 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?
There was a problem hiding this comment.
We'll just implement the small almost_eq method ; see "twice" in the example
#1384 (comment)
| auto boostFactors2 = new Real[numColumns]; | ||
| sp1.getBoostFactors(boostFactors1); | ||
| sp2.getBoostFactors(boostFactors2); | ||
| ASSERT_TRUE(check_vector_eq(boostFactors1, boostFactors2, numColumns)); |
There was a problem hiding this comment.
Another function is check_vector_eq(); used in two scenarios:
- comparing
array[]withvector<>. this could be avoided ei byvector.data() - for 2 vectors of
Real, it callsalmost_eqelement-wise; I'm not sure if that can be done implicitly without the need for such function?
There was a problem hiding this comment.
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)); |
There was a problem hiding this comment.
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(..)
|
Freeze. |
Fixes: #1383
Used for #1380