Compare network now returns a scalar #782
Conversation
Seanny123
force-pushed
the
scalar-compare
branch
from
87e5887
to
7f4429a
Compare
Seanny123
force-pushed
the
scalar-compare
branch
2 times, most recently
from
426976a
to
3022051
Compare
| actions = spa.Actions( | ||
| '0.5 --> motor=A', | ||
| 'dot(vision,CAT) --> motor=B', | ||
| 'dot(vision*CAT,DOG) --> motor=C', | ||
| '2*dot(vision,CAT*0.5) --> motor=D', | ||
| 'dot(vision,CAT)+0.5-dot(vision,CAT) --> motor=E', | ||
| 'dot(vision,PARROT) + compare --> motor=F', | ||
| 'dot(vision,MOUSE) + 0.5*compare --> motor=G', |
There was a problem hiding this comment.
I think you want 0.5 * dot(vision, MOUSE) + 0.5 * compare here.
| a scaling factor to be applied to the result | ||
| """ | ||
| output, _ = self.spa.get_module_output(source.name) | ||
| if(output.size_out != 1): |
There was a problem hiding this comment.
This should be if output.size_out != 1:
There was a problem hiding this comment.
What's the rule of thumb for adding parentheses to if-statements? It's not mentioned in the PEP8 and my personal preference is to just add them everywhere.
There was a problem hiding this comment.
I am okay with having parentheses. If I do add them though, I leave a space between the if and the rest of the conditional.
There was a problem hiding this comment.
I'm not okay with having parentheses, unless they're needed (for things like having an expression go across multiple lines). As far as I know we only use them in the nengo codebase when we need to go across multiple lines. Anyone know if I am wrong on that?
There was a problem hiding this comment.
A quick Google gave me nothing and flake8 didn't set off any alarms, so I guess it's personal? I'm fine with noting it down on our personal style guide or even putting it into our flake8 checks.
There was a problem hiding this comment.
Yeah. Parentheses and if statements don't work so well together. I do use them if the logic gets complex. Also, you can't use them to split lines because:
if (cond1 or
cond2):
do_things = True
Will throw a flake8 error (all the tabs are aligned).
There was a problem hiding this comment.
A quick Google gave me nothing and flake8 didn't set off any alarms, so I guess it's personal? I'm fine with noting it down on our personal style guide or even putting it into our flake8 checks.
Hmm, all of the single-line python if statements I can find (in nengo or in other places) do not use parentheses. Do you have examples where parentheses are used? I'm curious what they are...
There was a problem hiding this comment.
@tcstewar is right: no parentheses unless you need them. (The only exception I can think of is flag = (a == b); in that case I often use parentheses to make it easier to read.)
|
Nice. Other than the little things I commented on above, this looks nice to me, and could really simplify a lot of code. Thank you for doing this! |
| @@ -45,32 +45,23 @@ def __init__(self, dimensions, vocab=None, neurons_per_multiply=200, | |||
|
|
|||
| self.inputA = nengo.Node(size_in=dimensions, label='inputA') | |||
| self.inputB = nengo.Node(size_in=dimensions, label='inputB') | |||
| self.output = nengo.Node(size_in=dimensions, label='output') | |||
| self.output = nengo.Node(size_in=1, label='output') | |||
|
|
|||
| self.inputs = dict(A=(self.inputA, vocab), B=(self.inputB, vocab)) | |||
| self.outputs = dict(default=(self.output, vocab)) | |||
There was a problem hiding this comment.
Hmm, this output shouldn't have a vocab associated with it. I think the right thing to do would be self.outputs = dict(default=(self.output, None)) but I'm not sure if that breaks anything else....
jgosmann
force-pushed
the
scalar-compare
branch
from
a627ed7
to
d877109
Compare
|
Ditto here, worth putting in the changelog? At first glance it seems a bit esoteric to bother mentioning... |
|
Not sure, I was annoyed a lot by getting back some weird vector from compare. |
|
What's the heuristic as to what to put into the changelog and what to leave out? This does technically make an API change. |
|
Made a PR #804 with changelog entries added. |
Part of me thinks this the sort of thing that should go in the changelog, since it is an API change (and one that's not backwards-compatible). However, I'm pretty sure no one has used |
To fix #775 :
spa.Comparenetwork return a scalar.