-
Notifications
You must be signed in to change notification settings - Fork 5
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
Consolidate Encode, Decode, and Transcode #67
Conversation
Is it this a WIP to be discussed during a meeting due to the naming change? |
This is basically done except for what's not checked on the checklist or in the todo section. It seemed that there was consensus that this is a good idea when I did my nengo_spa presentation. |
nengo_spa/modules/transcode.py
Outdated
return fn | ||
else: | ||
raise ValidationError("Invalid output type {!r}".format( | ||
type(fn)), attr=self.name, obj=obj) | ||
|
||
def coerce_callable(self, obj, fn): | ||
t = 0. | ||
if self.sp_input: | ||
if obj.input_vocab is not None: | ||
print('a') |
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 think you accidentally left some print-statements in the code.
This looks really good to me. I like having all three as a single thing. I'm undecided on the name, but right now I'm leaning towards keeping While playing around it, I did find that a few more useful error messages would be handy. In particular: t1 = spa.Transcode('A') gives this error:
(I'd prefer something like "output_vocab must be specified") Similarly, this code gives an error at build time:
(I could understand this giving an error at build time if size_out has been specified, but when size_out is not specified the function does have to get evaluated to determine its output size and we should be able to detect that we're getting a string rather than a vector) |
Added a better error message for the two cases you mentioned. |
Name-wise, I vote for |
I think, I'm leaning more towards |
27b092e
to
8e8e983
Compare
@Seanny123 Can you approve the PR if there are no more outstanding issues? I think for know I will stick with |
(note: I rebased to master and force pushed.) |
Motivation and context:
This PR consolidates the Encode, Decode, and Transcode classes into a single Transcode class. The behaviour/expected function signature depends on what arguments of
input_vocab
,output_vocab
,size_in
andsize_out
have been set.I haven't decided on a final name for
Transcode
yet and suggestions are welcome. Because it is analogous to a Nengo nodeSpaNode
might work (or may even justNode
as we're doing it forNetwork
? Butspa.Network
can be used as a drop in replacement fornengo.Network
,Transcode
cannot be used as a drop in replacement fornengo.Node
).Interactions with other PRs:
none
How has this been tested?
existing tests
How long should this take to review?
Types of changes:
Checklist:
Still to do:
Transcode