-
Notifications
You must be signed in to change notification settings - Fork 75
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
An attempt at a base class for .py encoders #831
base: master
Are you sure you want to change the base?
Conversation
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 looks good 👍
let me know if you want to finish, or should I do sth?
import numpy as np | ||
from htm.bindings.sdr import SDR | ||
|
||
class BaseEncoder(ABC): |
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.
we've touched this..it's probably not worth creating a pybind interface for BaseEncoder.hpp? This file will be more flexible
|
||
@abstractmethod | ||
def reset(self): | ||
raise NotImplementedError() |
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 mistified you about the raise
, with @abstractmethod
you should replace the raise with just pass
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.
...and also remove the @AbstractMethod, most encoders dont use reset, so they'll use this implementation
def reset(self):
pass
@@ -20,8 +20,9 @@ | |||
|
|||
from htm.bindings.sdr import SDR | |||
from htm.bindings.math import Random | |||
from htm.bindings.encoder import BaseEncoder |
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.
the above is not in htm.bindings
class BaseEncoder(ABC): | ||
|
||
@abstractmethod | ||
def __init__(dimensions): |
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.
might think off of some common, useful params here: debug=0, inputType=None
,...
raise NotImplementedError() | ||
|
||
@abstractmethod | ||
def encode(self, input, output): |
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 one will remain abstract -> we want all subclasses to reimplement it.
But might add some common checks to save code elsewhere:
def encode(self, inp, output=None):
if output is None:
output = SDR(self.dimensions)
else:
assert( isinstance(output, SDR) )
assert( all(x == y for x, y in zip( output.dimensions, self.dimensions )))
if inp is None or (isinstance(inp, float) and math.isnan(inp)):
output.zero()
return output
@abstractmethod | ||
def __init__(dimensions): | ||
self.dimensions = dimensions | ||
self.size = SDR(dimensions).size |
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.
assert size > 0
@@ -60,14 +61,14 @@ def __init__(self, | |||
assert(self.sparsity <= 1) | |||
|
|||
# Assign each module a range of cells in the output SDR. | |||
partitions = np.linspace(0, self.size, num=len(self.periods) + 1) | |||
partitions = np.linspace(0, super().size, num=len(self.periods) + 1) |
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.
subclass has all the members,methods of its superclass, so this could/should remain self.size
, or even size
@@ -50,8 +51,8 @@ def __init__(self, | |||
encoder uses. This encoder produces deterministic output. The seed | |||
zero is special, seed zero is replaced with a truly random seed. | |||
""" | |||
self.size = size | |||
self.dimensions = (size,) | |||
super().__init__((size,)) |
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 correct use of explicitely calling the super's methods. (note: unlike in c++, you dont need to call the super.init as the first statement
@@ -95,10 +96,10 @@ def encode(self, location, grid_cells=None): | |||
location = list(location) |
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.
encode should call super().encode(location)
@breznak would you please finish this for me. Thanks. |
This is not complete...
..But I have to admit I don't know what I am doing. I am not proficient at Python to do the right thing here. @breznak can you help me?
What I am trying to do is develop a base class that can be used by any encoder developed in Python to ensure it implements the encode( ) function and sets the dimensions. I was trying to insert this into the existing grid_cell_encoder.py to make sure it works.