NUP-2506: Add test to all Serializable subclasses and fix related issues #3826
Conversation
return anomalyRegion | ||
|
||
|
||
def write(self, proto): | ||
proto.prevPredictedColumns = self.prevPredictedColumns.tolist() | ||
proto.prevPredictedColumns = self.prevPredictedColumns.ravel().tolist() |
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.
🤦♂️
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.
I had a couple comments but no blockers.
if self.currentOutput is not None: | ||
proto.currentOutput = self.currentOutput.tolist() | ||
if self.currentOutput is None: | ||
proto.currentOutput.none = None |
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.
I don't understand this. Is it a workaround to allow a proto to contain a null value? I see this pattern in several other places.
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 a way to handle 'None' in pycapnp. You first create a union with a 'Void' element representing 'None' and then initialize it with 'None'
see https://jparyani.github.io/pycapnp/quickstart.html#unions
@@ -126,7 +126,7 @@ def __init__(self, | |||
# If set to False, Cells4 will *not* be treated as an ephemeral member | |||
# and full BacktrackingTMCPP pickling is possible. This is useful for testing | |||
# pickle/unpickle without saving Cells4 to an external file | |||
self.makeCells4Ephemeral = True | |||
self.makeCells4Ephemeral = False |
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.
🤦♂️
actual = _getAttributes(serialized) | ||
|
||
# Make sure all fields were initialized | ||
self.assertEquals(actual, expected, klass.__name__) |
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 a great test.
encoder._prevAbsolute = proto.prevAbsolute | ||
encoder._prevDelta = proto.prevDelta | ||
encoder._prevAbsolute = None if proto.prevAbsolute == 0 else proto.prevAbsolute | ||
encoder._prevDelta = None if proto.prevDelta == 0 else proto.prevDelta |
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.
I personally don't think this is a bad change, but I'm curious if it will pass pylint.
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.
pylint
did not complain about this style
👍 Merge it and I'll release. |
@rhyolight Please review.
It should fix #3820, #3721, numenta/nupic.core-legacy#1065