-
Notifications
You must be signed in to change notification settings - Fork 1.6k
NUP-2396: Allow SensorRegion to pass actValue and bucketIdx to SDRClassifierRegion #3526
Conversation
cc @scottpurdy and @rhyolight |
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.
Done reviewing!
src/nupic/regions/RecordSensor.py
Outdated
if self.predictedFieldIdx > 0: | ||
fields = self.dataSource.getFieldNames() | ||
if self.predictedFieldIdx >= len(fields): | ||
raise ValueError("predictedFieldIdx (%s) must be strictly inferior " |
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.
"strictly inferior to the" -> "strictly less than the"
@@ -381,6 +402,22 @@ def compute(self, inputs, outputs): | |||
learn=self.learningMode, | |||
infer=False) | |||
|
|||
# If the input does not belong to a category, i.e. len(categories) == 0, | |||
# then look for bucketIdx and ActValueIn. |
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.
ActValueIn -> actValueIn
if len(categories) == 0: | ||
if "bucketIdxIn" not in inputs: | ||
raise KeyError("Network link missing: bucketIdxOut -> bucketIdxIn") | ||
if "actValue" not in inputs: |
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.
Shouldn't this be "actValueIn"?
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.
Good catch, thanks!
Also, can you include a test that exercises this behavior? One of my comments is on a line that looks like a bug so a test would help make sure all of this works as expected. |
Thanks for the review @scottpurdy To answer #3526 (comment), the "test" I am using to check that the new links work lives in this PR: #3520 Test usage: Would you rather have a unit test in nupic? |
Yes, a unit test with a simple network that exercises the new functionality would be great. It could be an sdr classifier unit test. |
…ensor and SDRClassifierRegion.
@scottpurdy thanks for the review. I integrated the feedback and also added unit tests. It's ready for review again. |
Fixes #3547 |
fb25c21
to
5cb9c71
Compare
This PR blocks #3520.
The network API does not support continuous inference at the moment. The workaround has been to call
customCompute()
on the classifier region to do inference.This PR fixes the issue: the change enables the creation of new links between sensor and classifier regions so that the
SensorRegion
can passbucketIdx
andactValue
to theSDRClassifierRegion
. This way inference can happen within the network, without having to callcustomCompute()
in an outer loop.