-
Notifications
You must be signed in to change notification settings - Fork 278
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
Fix SP unstable output, synapse decay #1382
Conversation
the unstable combination of constructor parameters can cause SP output to be different on the same input, which is clearly wrong!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please review, this provides test-cases for 2 identified problems with SP.
@@ -488,6 +488,18 @@ const vector<Real>& SpatialPooler::getBoostedOverlaps() const | |||
return boostedOverlaps_; | |||
} | |||
|
|||
/** helper method that checks SP output is stable for given configuration */ | |||
bool checkUnstableParams_(SpatialPooler &sp) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is the "stability check" that I suggest to run on each SP initialization. So far it has already identified many misconfigurations in our unit-tests.
/** helper method that checks SP output is stable for given configuration */ | ||
bool checkUnstableParams_(SpatialPooler &sp) { | ||
vector<UInt> input = {1,1,0,0,1,0}; //FIXME auto to size of SP's input | ||
if(sp.getInputDimensions().size()>1) return true; //FIXME skip nD as I cannot set auto input correctly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: I need some "automatic" input. And it must fit to the dimensions specified for the SP's input.
In most cases it's 1D and hard-coded value is sufficient, but for nD I'd need something as sp.getDemoInput()
; or is there any getter that has same dimensions as the input?
vector<UInt> out2(sp.getNumColumns(), 0); | ||
sp.compute(input.data(), true, out1.data()); | ||
sp.compute(input.data(), true, out2.data()); | ||
//TODO should we add SP.reset() and call it here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: now, the single run through the values is "stored" in SP's memory. Should we implement something as SP::reset()
(that just calls initialize with the same params) to avoid that?
@@ -593,6 +605,9 @@ void SpatialPooler::initialize(vector<UInt> inputDimensions, | |||
printParameters(); | |||
std::cout << "CPP SP seed = " << seed << std::endl; | |||
} | |||
|
|||
//check for reasonable params | |||
NTA_CHECK(checkUnstableParams_(*this)); //TODO the assert runs only at debug builds, make mandatory? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: make this mandatory even for Release builds?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently the check crashes? on some instances, while already identifies some problematic instances in our unit-tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.. crash fixed in #1391
different output for the same input. | ||
XXX sensitive - marks (empirically!) discovered set of parameter | ||
that produce the err behavior; changing any of the XXX params | ||
may cause the SP to behave "normally" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test-case for unstable combination of init params
{ | ||
/** this test exposes wrong behavior, where SP created via constructor | ||
behaves differently to a SP via setters (setXXX()), both with the same | ||
params. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Test case: different behavior when setters vs constructor is used for initialization
My patches cause some segfault, can you help me debug that please?
|
@scottpurdy @mrcslws @lscheinkman can you please have a look on the failing tests? (it's correct that the tests fail, that's the PR detecting hidden problems) @rhyolight A disadvantage with the circleCI is that I cannot link to specific lines of the build anymore (?) |
on each initialization, SP is tested to produce stable output.
2c0bdc0
to
c01a95b
Compare
as it's required
Update: the crash is fixed in #1391 and different params in #1387 , these 2 PRs need to be merged before work on this. TL;DR: Problem: cannot reproduce the diverging output as @scottpurdy reported in #1380 |
Closing as cannot reproduce. Waiting for feedback in the issue on how to reproduce. |
This PR adds tests for identified issue where c++ SP starts to misbehave under certain conditions and returns (random) different values for the same inputs.
Identified at least 2 cases
Fixes: #1380
Obsoletes: #1381
EDIT:
separate into several atomic PRs