numenta / nupic-legacy

Numenta Platform for Intelligent Computing is an implementation of Hierarchical Temporal Memory (HTM), a theory of intelligence based strictly on the neuroscience of the neocortex.
http://numenta.org/
MIT License
6.34k stars 1.56k forks source link

spatial_pooler.py: testUpdateMinDutyCycleLocal is faulty #1426

Open cogmission opened 10 years ago

cogmission commented 10 years ago

General Summary:

The testUpdateMinDutyCycleLocal() method sets up a mock which returns erroneous data, then the test corroborates that erroneous data as if it were supposed to be true!


Detailed Summary:

The testUpdateMinDutyCycleLocal() method sets up a mock in the form sp._getNeighborsND = Mock(side_effect=[[0, 1, 2], [1, 2, 3], [2, 3, 4], [0, 2, 4], [0, 1, 3]]) This mock instead returns [0, 0, 1, 2], [1, 1, 2, 3], [2, 2, 3, 4], [3, 0, 2, 4], [4, 0, 1, 3]

This also occurs for the second setup array within that test.


Problem:

The test grabs the "max" of the mock values specified by the mock indexes within the overlapDutyCycles array (i.e. for an overlap duty cycle of [1.2, 2.7, 0.9, 1.1, 4.3, 7.1, 2.3, 0.0], and using the zero'th index of the above mock ([0, 1, 2]), the max of that would be "2.7"). Which serendipitously works out when the mock returns [0, 0, 1, 2] instead of [0, 1, 2] because the inserted "0" is in the original set and so does not perturb the selection of the max value.

However, when the fourth index is used, the returned neighbor indexes are [4, 0, 1, 3] instead of simply [0, 1, 3] and as you cans see, the fourth index in the overlapDutyCycles array is "4.3" and will become the max returned.

The test simply tests that this "faulty" behavior occurs, because the test passes!


Solution:

This is not a problem during actual runtime conditions because as you can see at the bottom of spatial_pooler.py --> getNeighborsND() method, the actual method (which differs from the above test mock) removes the passed in index from the array of indexes returned.

However the test does not assert true behavior and so the mock return value should be fixed so that it extracts the array index from the returned value.

Most of all, this test will confound anyone trying to port this test to another language because the test results cannot be duplicated and will result in extreme confusion.

rhyolight commented 10 years ago

Assigned to @scottpurdy for review.

scottpurdy commented 10 years ago

Seems reasonable from the description.